Upgrade most dependencies and modernize codebase - #1363
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes migrate autograder processing from Que to Oban, replace Timex time operations with DateTime APIs across production and tests, update Oban schema/configuration, and apply assorted Phoenix, authentication, logging, dependency, schema, and unused-binding cleanups. ChangesDateTime time handling
Oban autograder migration
Platform and code cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the background job processing from Que to Oban, replaces Timex usage with Elixir's native DateTime and Date modules, and updates several dependencies. The review feedback identifies a critical security issue in the CORS configuration where compile-time checks cause origins to default to wildcard access, and a bug in the Oban worker failure handler that incorrectly marks failed jobs as completed. Additionally, the feedback suggests removing a redundant variable calculation in teams.ex and simplifying several DateTime.add/4 calls by using native time units directly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| cors_origins = | ||
| if Code.ensure_loaded?(__MODULE__) do | ||
| Application.compile_env(:cadet, :cors_endpoints, "*") | ||
| else | ||
| "*" | ||
| end |
There was a problem hiding this comment.
The module body code is executed at compile time. Since CadetWeb.Endpoint is not yet loaded during its own compilation, Code.ensure_loaded?(__MODULE__) will always evaluate to false at compile time. This causes cors_origins to always be hardcoded to "*", completely ignoring the configured :cors_endpoints in production. You should use Application.compile_env/3 directly.
cors_origins = Application.compile_env(:cadet, :cors_endpoints, "*")
There was a problem hiding this comment.
Resolved in 49dd427.
Removed the dead Code.ensure_loaded?(__MODULE__) guard (it always evaluated to false during the endpoint's own compilation, forcing origins to "*") and now read the value directly via Application.compile_env(:cadet, :cors_endpoints, "*").
One caveat worth flagging: I intentionally kept the top-level :cors_endpoints key rather than the [CadetWeb.Endpoint, :cors_endpoints] path where config/cadet.exs.example documents it. Production loads that config at runtime (config/releases.exs → /etc/cadet.exs), so reading the Endpoint key through compile_env/3 would trip Elixir's compile-vs-runtime consistency check and fail release boot. Behaviour is therefore unchanged from before (the old get_env(:cadet, [CadetWeb.Endpoint, :cors_endpoints], "*") also always returned "*", since get_env/3 doesn't support key paths). Actually wiring the runtime CORS value into Corsica is a separate, pre-existing concern.
| ids = Enum.map(team, &Map.get(&1, "userId")) | ||
|
|
||
| unique_ids_count = ids |> Enum.uniq() |> Enum.count() | ||
| _unique_ids_count = ids |> Enum.uniq() |> Enum.count() | ||
|
|
||
| student_already_in_team?(-1, ids, assessment_id) |
There was a problem hiding this comment.
The variable _unique_ids_count is calculated but never used. Since Enum.uniq/1 and Enum.count/1 are pure functions, this calculation is redundant and can be safely removed to improve performance and code clarity.
ids = Enum.map(team, &Map.get(&1, "userId"))
student_already_in_team?(-1, ids, assessment_id)
There was a problem hiding this comment.
Resolved in 7de83ce.
Removed the _unique_ids_count line entirely (pure computation with no side effects) rather than just underscore-prefixing it.
| if DateTime.compare( | ||
| cur_time, | ||
| DateTime.add(assessment.open_at, early_hours * 3_600, :second) | ||
| ) == | ||
| :lt do |
There was a problem hiding this comment.
Since Elixir 1.14, DateTime.add/4 supports :hour and other units directly. You can simplify early_hours * 3_600, :second to early_hours, :hour to make the code more readable and idiomatic.
if DateTime.compare(
cur_time,
DateTime.add(assessment.open_at, early_hours, :hour)
) ==
:lt do
There was a problem hiding this comment.
Resolved in 0ad414f.
Switched to DateTime.add(assessment.open_at, early_hours, :hour). (hours_before_early_xp_decay is an :integer, so the :hour unit is safe here.)
| DateTime.compare( | ||
| DateTime.add( | ||
| assessment.close_at, | ||
| voting_question.question["reveal_hours"] * 3_600, | ||
| :second | ||
| ), | ||
| DateTime.utc_now() | ||
| ) == :lt |
There was a problem hiding this comment.
Simplify the DateTime.add/4 call by using the :hour unit directly instead of multiplying by 3_600 seconds.
DateTime.compare(
DateTime.add(
assessment.close_at,
voting_question.question["reveal_hours"],
:hour
),
DateTime.utc_now()
) == :lt
There was a problem hiding this comment.
Resolved in 0ad414f.
Switched to the :hour unit directly. (reveal_hours is an :integer field.)
|
|
||
| def fetch_voting_questions_due_yesterday do | ||
| now = DateTime.utc_now() | ||
| yesterday = DateTime.add(now, -1 * 86_400, :second) |
There was a problem hiding this comment.
Simplify the DateTime.add/4 call by using the :day unit directly instead of multiplying by 86_400 seconds.
yesterday = DateTime.add(now, -1, :day)
There was a problem hiding this comment.
Resolved in 0ad414f.
Switched to DateTime.add(now, -1, :day).
| open_at = | ||
| Timex.now() | ||
| |> Timex.beginning_of_day() | ||
| |> Timex.shift(days: 3) | ||
| |> Timex.shift(hours: 4) | ||
| today | ||
| |> DateTime.new!(~T[00:00:00.000000]) | ||
| |> DateTime.add(3 * 86_400, :second) | ||
| |> DateTime.add(4 * 3_600, :second) | ||
|
|
||
| close_at = Timex.shift(open_at, days: 7) | ||
| close_at = DateTime.add(open_at, 7 * 86_400, :second) |
There was a problem hiding this comment.
Simplify the DateTime.add/4 calls by using :day and :hour units directly instead of multiplying by seconds.
open_at =
today
|> DateTime.new!(~T[00:00:00.000000])
|> DateTime.add(3, :day)
|> DateTime.add(4, :hour)
close_at = DateTime.add(open_at, 7, :day)
There was a problem hiding this comment.
Resolved in 0ad414f.
Switched to :day/:hour units for both open_at and close_at.
| overwrite: get_arg(args, :overwrite, false) | ||
| }) | ||
|
|
||
| :ok |
There was a problem hiding this comment.
In Oban, returning {:error, reason} is the standard way to report job failures. Returning :ok from handle_failure/4 will cause Oban to mark the job as completed rather than failed, which hides failures from Oban's monitoring and telemetry. Since max_attempts: 1 is configured, returning {:error, error_message} will correctly mark the job as failed without retrying.
{:error, error_message}
There was a problem hiding this comment.
Resolved in 9bd62e1.
handle_failure/4 now returns {:error, error_message} so Oban records the job as discarded (with max_attempts: 1) instead of completed, keeping the failure visible in Oban telemetry. The failed result is still enqueued to ResultStoreWorker before returning, so the answer is updated exactly as before, and the failure-handling test was updated to assert {:error, _}.
|
No base build to compare — deps-big-bang-2026 into master |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/cadet/assessments/assessment.ex (1)
91-97: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle missing
close_atto prevent runtime crashes.If a changeset is submitted with an
open_atvalue but a missing or invalidclose_atvalue, Ecto'sget_field(changeset, :close_at)will returnnil. PassingniltoDateTime.compare/2will raise aFunctionClauseError, resulting in a 500 Internal Server Error instead of returning a graceful validation failure.
lib/cadet/assessments/assessment.ex#L91-L97: Extractclose_at, add a nil-check beforeDateTime.compare, and invert the condition to safely apply the error.lib/cadet/stories/story.ex#L33-L39: Extractclose_at, add a nil-check beforeDateTime.compare, and invert the condition to safely apply the error.🛡️ Proposed fix (applies to both files)
validate_change(changeset, :open_at, fn :open_at, open_at -> - if DateTime.compare(open_at, get_field(changeset, :close_at)) == :lt do - [] - else - [open_at: "Open date must be before close date"] - end + close_at = get_field(changeset, :close_at) + if close_at && DateTime.compare(open_at, close_at) != :lt do + [open_at: "Open date must be before close date"] + else + [] + end end)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet/assessments/assessment.ex` around lines 91 - 97, Update the open_at validation in lib/cadet/assessments/assessment.ex lines 91-97 and the corresponding validation in lib/cadet/stories/story.ex lines 33-39: extract close_at, check that it is non-nil before calling DateTime.compare/2, and invert the condition so the existing error is returned when close_at is missing or open_at is not before it.lib/cadet_web/admin_controllers/admin_assessments_controller.ex (1)
145-158: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftCross-tenant authorization bypass (IDOR) on assessment modifications.
By explicitly mapping the URL's
course_idto an unused variable (_course_id), the controller assumes theassessment_idsafely belongs to the authenticated context without verifying it. A malicious or curious staff member of one course could supply anassessment_idthat belongs to another course, bypassing tenant segregation to trigger unauthorized score calculations or premature XP distributions.
lib/cadet_web/admin_controllers/admin_assessments_controller.ex#L145-L158: Bindcourse_idinstead of_course_idand wrap the core logic inside anis_same_course(course_id, assessment_id)check, returning a403 Forbiddenresponse if the validation fails.lib/cadet_web/admin_controllers/admin_assessments_controller.ex#L160-L174: Bindcourse_idinstead of_course_idand apply the exact sameis_same_course(course_id, assessment_id)ownership check before distributing XP.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet_web/admin_controllers/admin_assessments_controller.ex` around lines 145 - 158, Validate assessment ownership before performing either administrative operation. In lib/cadet_web/admin_controllers/admin_assessments_controller.ex lines 145-158, bind course_id and wrap calculate_contest_score’s existing logic in is_same_course(course_id, assessment_id), returning 403 Forbidden when it fails; apply the identical ownership check and course_id binding in lines 160-174 before distributing XP.
🧹 Nitpick comments (6)
lib/cadet_web/helpers/ai_comments_helpers.ex (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary variable assignment.
Since this is the last expression in the
ifblock, its result is implicitly returned. You can remove the_encrypted =assignment entirely.♻️ Proposed refactor
- _encrypted = - Base.encode64(iv) <> ":" <> Base.encode64(tag) <> ":" <> Base.encode64(ciphertext) + Base.encode64(iv) <> ":" <> Base.encode64(tag) <> ":" <> Base.encode64(ciphertext)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet_web/helpers/ai_comments_helpers.ex` around lines 60 - 61, Remove the unnecessary _encrypted assignment in the final expression of the if block, leaving the Base.encode64 concatenation as the block’s implicit return value.lib/cadet_web/controllers/generate_ai_comments.ex (1)
53-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused parameter from the function signature.
Since
_llm_api_keyis a private function parameter and is completely unused in the function body, consider removing it from the signature and updating its caller, rather than just prefixing it with an underscore.♻️ Proposed refactor
- defp check_llm_grading_parameters(_llm_api_key, llm_model, llm_api_url, llm_course_level_prompt) do + defp check_llm_grading_parameters(llm_model, llm_api_url, llm_course_level_prompt) do cond doMake sure to also update the caller at line 90:
- check_llm_grading_parameters( - key, - course.llm_model, - course.llm_api_url, - course.llm_course_level_prompt - ), + check_llm_grading_parameters( + course.llm_model, + course.llm_api_url, + course.llm_course_level_prompt + ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet_web/controllers/generate_ai_comments.ex` around lines 53 - 67, Remove the unused _llm_api_key argument from check_llm_grading_parameters and update its caller to pass only llm_model, llm_api_url, and llm_course_level_prompt. Preserve the existing validation behavior and return values.lib/cadet/accounts/teams.ex (1)
119-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused variable assignment.
Since
_unique_ids_countis unused and its computation has no side effects, you can remove this line entirely rather than just prefixing it with an underscore.♻️ Proposed refactor
- _unique_ids_count = ids |> Enum.uniq() |> Enum.count()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet/accounts/teams.ex` at line 119, Remove the unused _unique_ids_count assignment and its Enum.uniq/Enum.count computation from the surrounding function, leaving the remaining logic unchanged.test/cadet/updater/xml_parser_test.exs (1)
84-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
DateTime.new!/2over manual struct manipulation.Manually updating fields of a
DateTimestruct usingMap.put/3is non-idiomatic and bypasses validation. Since the goal is to get midnight of the current day in UTC, consider usingDate.utc_today/0andTimesigils, which exactly matches the approach used in the production code (XMLParser.process_assessment/3).
test/cadet/updater/xml_parser_test.exs#L84-L92: Replace theMap.putpipeline withDate.utc_today() |> DateTime.new!(~T[00:00:00.000000]).test/cadet_web/admin_controllers/admin_assessments_controller_test.exs#L467-L483: Replace theMap.putpipeline onnew_open_at.test/cadet_web/admin_controllers/admin_assessments_controller_test.exs#L500-L516: Replace theMap.putpipeline onnew_open_at.test/cadet_web/admin_controllers/admin_assessments_controller_test.exs#L532-L564: Replace theMap.putpipeline onopen_at.test/cadet_web/admin_controllers/admin_assessments_controller_test.exs#L584-L612: Replace theMap.putpipeline onopen_at.test/cadet_web/admin_controllers/admin_assessments_controller_test.exs#L632-L660: Replace theMap.putpipeline onopen_at.test/cadet_web/admin_controllers/admin_assessments_controller_test.exs#L680-L708: Replace theMap.putpipeline onopen_at.test/cadet_web/admin_controllers/admin_assessments_controller_test.exs#L728-L756: Replace theMap.putpipeline onopen_at.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cadet/updater/xml_parser_test.exs` around lines 84 - 92, Replace the manual DateTime Map.put pipelines with Date.utc_today() |> DateTime.new!(~T[00:00:00.000000]) in test/cadet/updater/xml_parser_test.exs:84-92 and each listed range in test/cadet_web/admin_controllers/admin_assessments_controller_test.exs (467-483, 500-516, 532-564, 584-612, 632-660, 680-708, 728-756), preserving the subsequent date arithmetic and using the existing open_at or new_open_at variables.lib/cadet/jobs/autograder/result_store_worker.ex (1)
24-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant public entrypoints:
perform/1(map clause) duplicatesrun/1.Same pattern as in
lib/cadet/jobs/autograder/lambda_worker.ex:perform(args) when is_map(args) and not is_struct(args)andrun/1do the same thing, giving two public ways to invoke the same logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet/jobs/autograder/result_store_worker.ex` around lines 24 - 40, Remove the redundant map-based perform/1 clause in the result-store worker and keep run/1 as the public direct-call entrypoint. Preserve the Oban.Worker perform/1 clause for Oban.Job inputs and its delegation to run/1.lib/cadet/jobs/autograder/lambda_worker.ex (1)
19-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant public entrypoints:
perform/1(map clause) duplicatesrun/1.
perform(args) when is_map(args) and not is_struct(args)andrun(args)do exactly the same thing. Consider dropping the map-argperform/1clause and having callers/tests userun/1directly, keepingperform/1solely as the Oban entrypoint.Separately, note that this bypass path (map-arg
perform/1/run/1) has no rescue/catch, unlike the%Oban.Job{}clause — any future caller invoking it directly outside of Oban won't get the graceful failure-enqueue behavior. This is currently safe because production code always dispatches throughOban.insert/1, but it's worth documenting explicitly rather than relying on the docstring's brief "bypass Oban" note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet/jobs/autograder/lambda_worker.ex` around lines 19 - 45, Remove the redundant map-argument perform/1 clause in the autograder worker, leaving perform/1 solely for %Oban.Job{} dispatch and directing direct callers/tests to run/1. Update the run/1 documentation to explicitly state that this bypass path does not apply the rescue/catch failure-enqueue handling used by the Oban entrypoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cadet_web/endpoint.ex`:
- Around line 5-14: Remove the Code.ensure_loaded?(__MODULE__) conditional from
the cors_origins definition in CadetWeb.Endpoint and read :cors_endpoints
directly with Application.compile_env(:cadet, :cors_endpoints, "*"). Keep the
existing "*" fallback and pass the resulting value to Corsica unchanged.
---
Outside diff comments:
In `@lib/cadet_web/admin_controllers/admin_assessments_controller.ex`:
- Around line 145-158: Validate assessment ownership before performing either
administrative operation. In
lib/cadet_web/admin_controllers/admin_assessments_controller.ex lines 145-158,
bind course_id and wrap calculate_contest_score’s existing logic in
is_same_course(course_id, assessment_id), returning 403 Forbidden when it fails;
apply the identical ownership check and course_id binding in lines 160-174
before distributing XP.
In `@lib/cadet/assessments/assessment.ex`:
- Around line 91-97: Update the open_at validation in
lib/cadet/assessments/assessment.ex lines 91-97 and the corresponding validation
in lib/cadet/stories/story.ex lines 33-39: extract close_at, check that it is
non-nil before calling DateTime.compare/2, and invert the condition so the
existing error is returned when close_at is missing or open_at is not before it.
---
Nitpick comments:
In `@lib/cadet_web/controllers/generate_ai_comments.ex`:
- Around line 53-67: Remove the unused _llm_api_key argument from
check_llm_grading_parameters and update its caller to pass only llm_model,
llm_api_url, and llm_course_level_prompt. Preserve the existing validation
behavior and return values.
In `@lib/cadet_web/helpers/ai_comments_helpers.ex`:
- Around line 60-61: Remove the unnecessary _encrypted assignment in the final
expression of the if block, leaving the Base.encode64 concatenation as the
block’s implicit return value.
In `@lib/cadet/accounts/teams.ex`:
- Line 119: Remove the unused _unique_ids_count assignment and its
Enum.uniq/Enum.count computation from the surrounding function, leaving the
remaining logic unchanged.
In `@lib/cadet/jobs/autograder/lambda_worker.ex`:
- Around line 19-45: Remove the redundant map-argument perform/1 clause in the
autograder worker, leaving perform/1 solely for %Oban.Job{} dispatch and
directing direct callers/tests to run/1. Update the run/1 documentation to
explicitly state that this bypass path does not apply the rescue/catch
failure-enqueue handling used by the Oban entrypoint.
In `@lib/cadet/jobs/autograder/result_store_worker.ex`:
- Around line 24-40: Remove the redundant map-based perform/1 clause in the
result-store worker and keep run/1 as the public direct-call entrypoint.
Preserve the Oban.Worker perform/1 clause for Oban.Job inputs and its delegation
to run/1.
In `@test/cadet/updater/xml_parser_test.exs`:
- Around line 84-92: Replace the manual DateTime Map.put pipelines with
Date.utc_today() |> DateTime.new!(~T[00:00:00.000000]) in
test/cadet/updater/xml_parser_test.exs:84-92 and each listed range in
test/cadet_web/admin_controllers/admin_assessments_controller_test.exs (467-483,
500-516, 532-564, 584-612, 632-660, 680-708, 728-756), preserving the subsequent
date arithmetic and using the existing open_at or new_open_at variables.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfd6ae81-c62f-4eda-a23f-65447f7a111a
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
config/config.exslib/cadet/accounts/teams.exlib/cadet/application.exlib/cadet/assessments/assessment.exlib/cadet/assessments/assessments.exlib/cadet/assessments/version_manager.exlib/cadet/auth/guardian.exlib/cadet/auth/providers/openid/nus_entra_id_claim_extractor.exlib/cadet/chatbot/llm_conversations.exlib/cadet/code_exchange.exlib/cadet/helpers/model_helper.exlib/cadet/jobs/autograder/grading_job.exlib/cadet/jobs/autograder/lambda_worker.exlib/cadet/jobs/autograder/result_store_worker.exlib/cadet/jobs/autograder/utilities.exlib/cadet/jobs/log.exlib/cadet/jobs/xml_parser.exlib/cadet/logger/cloudwatch_logger.exlib/cadet/stories/stories.exlib/cadet/stories/story.exlib/cadet_web.exlib/cadet_web/admin_controllers/admin_assessments_controller.exlib/cadet_web/admin_controllers/admin_teams_controller.exlib/cadet_web/admin_views/admin_grading_view.exlib/cadet_web/controllers/assessments_controller.exlib/cadet_web/controllers/auth_controller.exlib/cadet_web/controllers/generate_ai_comments.exlib/cadet_web/endpoint.exlib/cadet_web/helpers/ai_comments_helpers.exlib/cadet_web/helpers/view_helper.exmix.exspriv/repo/migrations/20230214140555_create_notification_preferences.exspriv/repo/migrations/20260715000000_update_oban_to_v14.exstest/cadet/assessments/assessment_test.exstest/cadet/assessments/assessments_test.exstest/cadet/jobs/autograder/grading_job_test.exstest/cadet/jobs/autograder/lambda_worker_test.exstest/cadet/jobs/autograder/result_store_worker_test.exstest/cadet/jobs/autograder/utilities_test.exstest/cadet/jobs/log_test.exstest/cadet/stories/stories_test.exstest/cadet/updater/xml_parser_test.exstest/cadet_web/admin_controllers/admin_assessments_controller_test.exstest/cadet_web/admin_controllers/admin_grading_controller_test.exstest/cadet_web/admin_controllers/admin_stories_controller_test.exstest/cadet_web/controllers/answer_controller_test.exstest/cadet_web/controllers/assessments_controller_test.exstest/cadet_web/controllers/stories_controller_test.exstest/factories/assessments/assessment_factory.extest/factories/stories/story_factory.ex
💤 Files with no reviewable changes (2)
- lib/cadet/chatbot/llm_conversations.ex
- lib/cadet/assessments/version_manager.ex
Re: CodeRabbit review — resolutionsAddressing the findings that were posted in the review body (outside the diff range) and the nitpicks, since those don't have inline threads to reply to. Outside diff range comments
Nitpick comments
Intentionally not changed
All changes verified locally: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/cadet_web/admin_controllers/admin_assessments_controller.ex (1)
189-189: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid parsing
nildates in partial date edits.
openAt/closeAtcan be omitted, butDateTime.from_iso8601(nil)raises beforecheck_datescan return a bad request. Parse each supplied date independently, fall back to the persisted date when omitted, and only compare when both values are present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cadet_web/admin_controllers/admin_assessments_controller.ex` at line 189, Update the date handling around check_dates so openAt and closeAt are parsed independently only when supplied, while omitted values reuse their persisted dates. Ensure nil values do not reach DateTime.from_iso8601, and perform the DateTime.compare check only when both dates are present so partial edits can return the intended bad request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/cadet_web/admin_controllers/admin_assessments_controller.ex`:
- Line 189: Update the date handling around check_dates so openAt and closeAt
are parsed independently only when supplied, while omitted values reuse their
persisted dates. Ensure nil values do not reach DateTime.from_iso8601, and
perform the DateTime.compare check only when both dates are present so partial
edits can return the intended bad request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eb65cc0c-bbf4-4d65-95a0-144cf1152643
📒 Files selected for processing (13)
lib/cadet/accounts/teams.exlib/cadet/assessments/assessment.exlib/cadet/assessments/assessments.exlib/cadet/jobs/autograder/lambda_worker.exlib/cadet/jobs/xml_parser.exlib/cadet/stories/story.exlib/cadet_web/admin_controllers/admin_assessments_controller.exlib/cadet_web/controllers/generate_ai_comments.exlib/cadet_web/endpoint.exlib/cadet_web/helpers/ai_comments_helpers.extest/cadet/jobs/autograder/lambda_worker_test.exstest/cadet/updater/xml_parser_test.exstest/cadet_web/admin_controllers/admin_assessments_controller_test.exs
💤 Files with no reviewable changes (1)
- lib/cadet/accounts/teams.ex
🚧 Files skipped from review as they are similar to previous changes (11)
- lib/cadet_web/endpoint.ex
- lib/cadet_web/helpers/ai_comments_helpers.ex
- lib/cadet/stories/story.ex
- lib/cadet/assessments/assessment.ex
- test/cadet/updater/xml_parser_test.exs
- lib/cadet_web/controllers/generate_ai_comments.ex
- lib/cadet/jobs/xml_parser.ex
- test/cadet_web/admin_controllers/admin_assessments_controller_test.exs
- test/cadet/jobs/autograder/lambda_worker_test.exs
- lib/cadet/jobs/autograder/lambda_worker.ex
- lib/cadet/assessments/assessments.ex
Failure scenario: with |
The Supervisor.Spec -> child-spec-tuple migration changed
`worker(OpenIDConnect.Worker, [providers])` into
`{OpenIDConnect.Worker, [providers]}`, which double-wraps the argument.
`worker(mod, [providers])` set the start MFA to
`{mod, :start_link, [providers]}`, i.e. `start_link(providers)`. The
child-spec tuple `{mod, arg}` instead invokes `mod.child_spec(arg)`,
whose default (from `use GenServer`) is `{mod, :start_link, [arg]}` ->
`start_link([providers])`.
OpenIDConnect.Worker.init/1 does
`Enum.into(provider_configs, %{}, fn {provider, config} -> ... end)`.
With `[providers]` the single element is the whole providers keyword
list, so the `{provider, config}` clause raises FunctionClauseError,
the child never starts, and (where openid_connect_providers is
configured, i.e. prod) the supervisor exhausts restarts and boot fails.
Pass `providers` directly so child_spec resolves to
`start_link(providers)`, exactly matching the pre-migration behaviour.
Reported in review: PR #1363.
|
I found three issues in the current PR head (
The current-head CI passes formatting, Credo, migrations, tests, and Dialyzer. |
`cors_endpoints` is configured beneath `CadetWeb.Endpoint` (see
`config/cadet.exs.example`), but the endpoint read the top-level
`:cors_endpoints` key, so it always fell back to `"*"`. Combined with
`allow_credentials: true`, Corsica then echoes back whatever `Origin` the
request carries rather than sending a literal `*`:
send_wildcard_origin?(%Options{origins: origins, allow_credentials: c}) ->
origins == "*" and not c
so every origin is allowed to make credentialed requests.
Reading `[CadetWeb.Endpoint, :cors_endpoints]` fixes the path, but not
via `Application.compile_env/3`: this app is deployed as a Mix release
and `config/releases.exs` evaluates `/etc/cadet.exs` at boot, so
`cors_endpoints` is absent at build time and present at runtime. Releases
default to `validate_compile_env: true`, which compares the two and
aborts:
the application :cadet has a different value set for path
[:cors_endpoints] inside key CadetWeb.Endpoint during runtime
compared to compile time.
Endpoint plugs are also initialised at compile time
(`Phoenix.plug_init_mode/0` defaults to `:compile`, and this app never
sets it), so the value has to be read outside `init/1` regardless. Move
Corsica behind a small plug that resolves the origins on first call and
caches the parsed options in `:persistent_term`.
Verified by setting `:cors_endpoints` after compilation and driving a
preflight request through the endpoint: the configured origin gets
`access-control-allow-origin`, others get no CORS headers at all; with
nothing configured the previous `"*"` behaviour is unchanged.
Reported in review: PR #1363.
`Oban.Migrations.down/1` is inclusive of the version it is given:
def down(opts) do
...
if initial >= opts.version do
change(initial..opts.version//-1, :down, opts)
end
end
and `change/3` then records `Enum.min(range) - 1` as the new schema
version. So `down(version: 11)` runs V14..V11 down and leaves the
database at v10, dropping the `oban_peers` table that V11 creates.
The preceding migration (20230214081421) migrates up to v11, so rolling
back this migration should land on v11, not v10 -- otherwise a
deployment rollback leaves the previous release running against a schema
missing `oban_peers`.
Confirmed against a live database. With `down(version: 11)` a rollback
gives schema version 10 and `to_regclass('public.oban_peers')` is NULL;
with `down(version: 12)` it gives 11 with `oban_peers` intact.
Reported in review: PR #1363.
`ResultStoreWorker` was `use Que.Worker, concurrency: 5` before the move
to Oban, deliberately separate from the grading work so that database
writes stayed bounded -- as its `@moduledoc` still says:
Separate worker is created with lower concurrency on the assumption
that autograding time >> db IO time so as to reduce db load.
Putting it on the same `autograder: 20` queue as `LambdaWorker` lost
both halves of that: result jobs now queue behind a batch's Lambda
invocations, and up to 20 of them can then write to the database at
once.
Restore the separation with a dedicated `autograder_results: 5` queue.
Reported in review: PR #1363.
RichDom2185
changed the title
|
All three findings are valid — but one of the suggested fixes was wrong and would have broken production. Verdicts1. CORS wrong config path (P1) — valid diagnosis, unsafe fix
The suggested
That's the same class of bug as 2. Oban rollback drops schema v11 (P1) — valid, fixed as suggested
3. Result storage lost its concurrency limit (P2) — valid, fixed as suggested Pre-Oban it was VerificationEverything CI runs, locally: format ✓, credo ✓ (exit 0), migrate + rollback ✓, One note: your local I did not reply on the PR — say the word if you'd like me to post the assessment, particularly the pushback on the |
Remove the dead `Code.ensure_loaded?(__MODULE__)` guard, which always evaluated to false during the endpoint's own compilation and forced `origins` to "*". Read the config directly with compile_env. Addresses PR review comments.
DateTime.add/4 supports :day and :hour units directly (Elixir >= 1.14), so drop the manual second multiplications for readability. Addresses PR review comments.
- teams.ex: drop the unused unique-id count (pure computation, no effect) - ai_comments_helpers.ex: return the encrypted string directly - generate_ai_comments.ex: drop the unused api-key parameter and its caller arg Addresses PR review comments.
handle_failure/4 returned :ok, which made Oban mark the job as completed
even though the Lambda invocation failed. Return {:error, message} so the
job is recorded as discarded (max_attempts: 1) and stays visible in Oban
telemetry. The failed result is still enqueued beforehand, so the answer
is updated as before.
Addresses PR review comments.
calculate_contest_score/2 and dispatch_contest_xp/2 looked up the voting question by assessment id only, ignoring the course id in the path. A staff member of one course could therefore trigger score calculation or XP dispatch on another course's assessment. Guard both with the existing is_same_course/2 check (returning 403 on mismatch), matching delete/2, and extract the shared lookup into a helper. Addresses PR review comments.
get_field(changeset, :close_at) can be nil (e.g. missing on insert), which made DateTime.compare/2 raise a FunctionClauseError and surface as a 500 instead of a validation error. Skip the comparison when close_at is nil and let validate_required/2 report the missing field. Addresses PR review comments.
Build midnight-today with Date.utc_today() |> DateTime.new!/2 instead of mutating a DateTime via Map.put/3, matching XMLParser.process_assessment/3. Addresses PR review comments.
The Timex -> DateTime migration expressed date offsets as magic-number second arithmetic (e.g. `N * 86_400, :second`), while a few call sites already used the clearer `:day`/`:hour` units that `DateTime.add/4` supports. Normalize the rest to match, restoring the units the original Timex code expressed (days:/hours:/from_minutes): * `N * 86_400, :second` -> `N, :day` * `N * 3_600, :second` -> `N, :hour` * `-25 * 60 * 60, :second` -> `-25, :hour` * `-86_400, :second` -> `-1, :day` * `-60, :second` -> `-1, :minute` `DateTime.add/4` treats these units as fixed multiples (86400/3600/60 seconds), so the values are exactly equivalent. Genuinely second-valued call sites (period_seconds, code_ttl, truncate) are left unchanged.
The Supervisor.Spec -> child-spec-tuple migration changed
`worker(OpenIDConnect.Worker, [providers])` into
`{OpenIDConnect.Worker, [providers]}`, which double-wraps the argument.
`worker(mod, [providers])` set the start MFA to
`{mod, :start_link, [providers]}`, i.e. `start_link(providers)`. The
child-spec tuple `{mod, arg}` instead invokes `mod.child_spec(arg)`,
whose default (from `use GenServer`) is `{mod, :start_link, [arg]}` ->
`start_link([providers])`.
OpenIDConnect.Worker.init/1 does
`Enum.into(provider_configs, %{}, fn {provider, config} -> ... end)`.
With `[providers]` the single element is the whole providers keyword
list, so the `{provider, config}` clause raises FunctionClauseError,
the child never starts, and (where openid_connect_providers is
configured, i.e. prod) the supervisor exhausts restarts and boot fails.
Pass `providers` directly so child_spec resolves to
`start_link(providers)`, exactly matching the pre-migration behaviour.
Reported in review: PR #1363.
`cors_endpoints` is configured beneath `CadetWeb.Endpoint` (see
`config/cadet.exs.example`), but the endpoint read the top-level
`:cors_endpoints` key, so it always fell back to `"*"`. Combined with
`allow_credentials: true`, Corsica then echoes back whatever `Origin` the
request carries rather than sending a literal `*`:
send_wildcard_origin?(%Options{origins: origins, allow_credentials: c}) ->
origins == "*" and not c
so every origin is allowed to make credentialed requests.
Reading `[CadetWeb.Endpoint, :cors_endpoints]` fixes the path, but not
via `Application.compile_env/3`: this app is deployed as a Mix release
and `config/releases.exs` evaluates `/etc/cadet.exs` at boot, so
`cors_endpoints` is absent at build time and present at runtime. Releases
default to `validate_compile_env: true`, which compares the two and
aborts:
the application :cadet has a different value set for path
[:cors_endpoints] inside key CadetWeb.Endpoint during runtime
compared to compile time.
Endpoint plugs are also initialised at compile time
(`Phoenix.plug_init_mode/0` defaults to `:compile`, and this app never
sets it), so the value has to be read outside `init/1` regardless. Move
Corsica behind a small plug that resolves the origins on first call and
caches the parsed options in `:persistent_term`.
Verified by setting `:cors_endpoints` after compilation and driving a
preflight request through the endpoint: the configured origin gets
`access-control-allow-origin`, others get no CORS headers at all; with
nothing configured the previous `"*"` behaviour is unchanged.
Reported in review: PR #1363.
`Oban.Migrations.down/1` is inclusive of the version it is given:
def down(opts) do
...
if initial >= opts.version do
change(initial..opts.version//-1, :down, opts)
end
end
and `change/3` then records `Enum.min(range) - 1` as the new schema
version. So `down(version: 11)` runs V14..V11 down and leaves the
database at v10, dropping the `oban_peers` table that V11 creates.
The preceding migration (20230214081421) migrates up to v11, so rolling
back this migration should land on v11, not v10 -- otherwise a
deployment rollback leaves the previous release running against a schema
missing `oban_peers`.
Confirmed against a live database. With `down(version: 11)` a rollback
gives schema version 10 and `to_regclass('public.oban_peers')` is NULL;
with `down(version: 12)` it gives 11 with `oban_peers` intact.
Reported in review: PR #1363.
`ResultStoreWorker` was `use Que.Worker, concurrency: 5` before the move
to Oban, deliberately separate from the grading work so that database
writes stayed bounded -- as its `@moduledoc` still says:
Separate worker is created with lower concurrency on the assumption
that autograding time >> db IO time so as to reduce db load.
Putting it on the same `autograder: 20` queue as `LambdaWorker` lost
both halves of that: result jobs now queue behind a batch's Lambda
invocations, and up to 20 of them can then write to the database at
once.
Restore the separation with a dedicated `autograder_results: 5` queue.
Reported in review: PR #1363.
16cc7e5 to
32fddf6
Compare
sayomaki
left a comment
sayomaki
left a comment
There was a problem hiding this comment.
LGTM, although claude made a note on lib/cadet/jobs/autograder/lambda_worker.ex:30 where manual invocations of run will not handle errors gracefully and report them and fail silently. This is not an issue for prod as it will always call the right function.
3 participants
2026 maintenance update to maintain compatibility with newer Elixir versions to come
This is part 1 of 3 in a stack made with GitButler: