Add support for conductor assessments - #1367
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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
WalkthroughChangesCore application changes
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 |
|
coverage: 87.393% (-0.07%) from 87.459% — conductor-assessments into master |
dc5f5db to
0e8a4da
Compare
3394f2a to
2e5e295
Compare
2e5e295 to
3999d4b
Compare
RichDom2185
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
.credo.exs (1)
97-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the global complexity limit at 11 unless a current violation requires 16.
Line 97 raises the limit for every analyzed module. The supplied
credo-original.txtandcredo-branch.txtreports contain noCyclomaticComplexityfinding. Run Credo with the limit set to 11 on this branch. If no violation appears, revert this change. Otherwise, refactor the specific function or document a scoped exception.Suggested default
- {Credo.Check.Refactor.CyclomaticComplexity, max_complexity: 16}, + {Credo.Check.Refactor.CyclomaticComplexity, max_complexity: 11},🤖 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 @.credo.exs at line 97, Revert the global max_complexity change in the Credo configuration to 11 unless running Credo at that limit reveals a current violation. If a violation exists, refactor the identified function or apply a narrowly scoped exception instead of raising the limit for all modules.test/support/xml_generator.ex (1)
225-233: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLegacy
library_attrsdropsvariantandexectime.
programminglanguage/2andgraderprogramminglanguage/2now permitvariantandexectimeattributes, butlibrary_attrs/1for the legacy (non-conductor) clause only returns%{interpreter: library.chapter}. LegacyLibrarystructs carryvariantandexec_time_ms. As written, no test built throughprocess_library/2can generate legacy XML withvariant/exectimeattributes, even though the tag now supports them.Add
variantandexectimeto the legacylibrary_attrs/1clause so tests can exercise variant/exectime round-tripping through the generator.♻️ Proposed fix
defp library_attrs(library) do - %{interpreter: library.chapter} + %{interpreter: library.chapter, variant: library[:variant], exectime: library[:exec_time_ms]} + |> Map.reject(fn {_k, v} -> is_nil(v) 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 `@test/support/xml_generator.ex` around lines 225 - 233, Update the legacy `library_attrs/1` clause to include the `variant` and `exectime` XML attributes, mapping them from the legacy `Library` fields `variant` and `exec_time_ms` while retaining the existing `interpreter: library.chapter` attribute.lib/cadet/jobs/xml_parser.ex (1)
296-331: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAllow mixed
library/grading_libraryformats if supported.
parse_programming_language/1can produce different:formatvalues for the two fields, and LambdaWorker routesquestion.grading_libraryby its own:format. Keep this parsing behavior if mixed legacy/conductor values are valid, or add an explicit validation rule that rejects it.🤖 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/xml_parser.ex` around lines 296 - 331, Update process_question_library/3 to explicitly handle mixed formats between the parsed library and grading_library values: preserve both results when mixed legacy/conductor formats are supported, or add validation that returns an error when their :format values differ. Ensure the behavior aligns with LambdaWorker routing grading_library by its own format.lib/cadet/jobs/autograder/result_store_worker.ex (1)
6-8: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider retries for the result-persistence queue.
max_attempts: 1discards the job after a single transient database error, so the autograding result is lost and the answer keepsautograding_status: :processing. The write path is idempotent peranswer_id, so a smallmax_attemptswith backoff is safe and recovers from short database outages.🤖 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 6 - 8, Update the Oban.Worker configuration for the result-persistence worker to use a small retry count instead of max_attempts: 1, preserving the idempotent answer_id write path and enabling Oban’s backoff to recover from transient database errors.test/cadet_web/controllers/stories_controller_test.exs (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the leftover
use Timex.This file no longer calls any Timex function after the migration. The
use Timexdirective at Line 3 is now dead setup and keeps this test coupled to Timex.🤖 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_web/controllers/stories_controller_test.exs` around lines 13 - 14, Remove the unused use Timex directive from the test module, leaving the DateTime-based setup and remaining test configuration unchanged.lib/cadet/jobs/autograder/lambda_worker.ex (1)
122-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
get_arg/3to a shared helper.
Cadet.Autograder.ResultStoreWorkerdefines the identical privateget_arg/3at lines 80-82 oflib/cadet/jobs/autograder/result_store_worker.ex. That module already importsCadet.SharedHelper. Put one implementation there and import it in both workers.🤖 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 122 - 124, Move the duplicated get_arg/3 implementation from the worker modules into Cadet.SharedHelper, then import or otherwise expose that shared helper in both Cadet.Autograder.LambdaWorker and Cadet.Autograder.ResultStoreWorker. Remove each worker’s private definition while preserving the existing key and default lookup behavior.lib/cadet/jobs/autograder/utilities.ex (1)
16-38: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnqueue the job in the same transaction as the status update.
Repo.update!/1commitsautograding_status: :processingbeforeOban.insert/1runs. If the insert fails, or the process stops between the two operations, the answer stays:processingand is never regraded, becausegrade_submission_question_answer_lists/5only regrades answers with status:noneor:failed. Wrap both operations in oneEcto.Multiand useOban.insert/3with the multi so the status change rolls back with a failed enqueue.🤖 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/utilities.ex` around lines 16 - 38, Update dispatch_programming_answer/3 to execute the Answer.autograding_changeset update and Oban enqueue within a single Ecto.Multi transaction, replacing the standalone Repo.update!/1 and Oban.insert/1 calls. Use Oban.insert/3 with the multi so any enqueue failure rolls back the :processing status update, while preserving the existing job arguments and overwrite behavior.
🤖 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 @.mcp.json:
- Line 7: Remove the hardcoded CONTEXT7_API_KEY value from .mcp.json, revoke and
rotate the exposed credential, purge it from Git history, and configure the MCP
client to obtain the key through its supported secret mechanism instead.
In `@lib/cadet_web/admin_controllers/admin_teams_controller.ex`:
- Around line 93-94: Update the course-scoped delete clause in
AdminTeamsController.delete to load the team through its assessment and validate
that assessment.course_id matches the requested course_id before deletion.
Return the existing 404/403 response when the team is missing or belongs to
another course, and only delegate to the team-id deletion path after this
authorization check.
In `@lib/cadet_web/controllers/generate_ai_comments.ex`:
- Line 53: Update check_llm_grading_parameters/3 to validate that the decrypted
LLM API key is non-empty before allowing the grading request to proceed. Return
the existing configuration-error result for an empty key, while preserving the
current behavior for valid keys and other parameter checks.
In `@lib/cadet_web/plug/cors.ex`:
- Around line 42-44: Update the CORS configuration lookup in the endpoint
settings flow to require an explicit :cors_endpoints value instead of defaulting
to "*". Reject "*" when credentialed CORS is enabled, while preserving valid
explicit origin-list handling and failing closed for missing or invalid
configuration.
In `@lib/cadet/jobs/autograder/lambda_worker.ex`:
- Around line 75-79: Update enqueue_result_store/1 to propagate the result of
Oban.insert/1 instead of discarding it, and make run_with_models/1 return that
result rather than unconditionally returning :ok. Apply the same result handling
to the enqueue_result_store/1 call in handle_failure/4 so insertion errors
propagate in both success and failure paths.
In `@lib/cadet/jobs/autograder/result_store_worker.ex`:
- Around line 67-78: Add fallback handling in normalize_status/1 for unknown
string or other status values, mapping them to a safe default status instead of
raising and leaving the answer in :processing; also add a non-map clause in
normalize_result/1 so nil results are normalized without crashing. Preserve the
existing mappings for "success", "failed", and atom statuses.
In `@lib/cadet/jobs/xml_parser.ex`:
- Around line 63-81: Update extract_changeset_error_message to traverse nested
embed errors via Ecto.Changeset.traverse_errors/2, including validation messages
from embedded Library changesets for :library and :grading_library instead of
reducing them to the generic "embed invalid" text. Preserve the existing
formatting and stringification behavior for top-level errors.
In `@test/cadet/updater/xml_parser_test.exs`:
- Around line 280-310: Update the “Conductor programming language” tests to
cover distinct XML paths: construct the TASK-level fixture using the XML
generator’s library: and grading_library: options, and give the per-PROBLEM
override fixture a different library. After XMLParser.parse_xml, assert the
persisted task-level and problem-level library values so each path is verified
independently.
---
Nitpick comments:
In @.credo.exs:
- Line 97: Revert the global max_complexity change in the Credo configuration to
11 unless running Credo at that limit reveals a current violation. If a
violation exists, refactor the identified function or apply a narrowly scoped
exception instead of raising the limit for all modules.
In `@lib/cadet/jobs/autograder/lambda_worker.ex`:
- Around line 122-124: Move the duplicated get_arg/3 implementation from the
worker modules into Cadet.SharedHelper, then import or otherwise expose that
shared helper in both Cadet.Autograder.LambdaWorker and
Cadet.Autograder.ResultStoreWorker. Remove each worker’s private definition
while preserving the existing key and default lookup behavior.
In `@lib/cadet/jobs/autograder/result_store_worker.ex`:
- Around line 6-8: Update the Oban.Worker configuration for the
result-persistence worker to use a small retry count instead of max_attempts: 1,
preserving the idempotent answer_id write path and enabling Oban’s backoff to
recover from transient database errors.
In `@lib/cadet/jobs/autograder/utilities.ex`:
- Around line 16-38: Update dispatch_programming_answer/3 to execute the
Answer.autograding_changeset update and Oban enqueue within a single Ecto.Multi
transaction, replacing the standalone Repo.update!/1 and Oban.insert/1 calls.
Use Oban.insert/3 with the multi so any enqueue failure rolls back the
:processing status update, while preserving the existing job arguments and
overwrite behavior.
In `@lib/cadet/jobs/xml_parser.ex`:
- Around line 296-331: Update process_question_library/3 to explicitly handle
mixed formats between the parsed library and grading_library values: preserve
both results when mixed legacy/conductor formats are supported, or add
validation that returns an error when their :format values differ. Ensure the
behavior aligns with LambdaWorker routing grading_library by its own format.
In `@test/cadet_web/controllers/stories_controller_test.exs`:
- Around line 13-14: Remove the unused use Timex directive from the test module,
leaving the DateTime-based setup and remaining test configuration unchanged.
In `@test/support/xml_generator.ex`:
- Around line 225-233: Update the legacy `library_attrs/1` clause to include the
`variant` and `exectime` XML attributes, mapping them from the legacy `Library`
fields `variant` and `exec_time_ms` while retaining the existing `interpreter:
library.chapter` attribute.
🪄 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 Plus
Run ID: 2a61d9f9-4b39-498d-bd1a-14441c0a266d
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.credo.exs.mcp.jsonconfig/config.exscredo-branch.txtcredo-original.txtlib/cadet/accounts/teams.exlib/cadet/application.exlib/cadet/assessments/assessment.exlib/cadet/assessments/assessments.exlib/cadet/assessments/library.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/assessments_helpers.exlib/cadet_web/helpers/view_helper.exlib/cadet_web/plug/cors.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/assessments/library_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/assessments/library_factory.extest/factories/stories/story_factory.extest/support/xml_generator.ex
💤 Files with no reviewable changes (1)
- lib/cadet/assessments/version_manager.ex
| "type": "http", | ||
| "url": "https://mcp.context7.com/mcp", | ||
| "headers": { | ||
| "CONTEXT7_API_KEY": "ctx7sk-9324f560-42a4-4383-a942-ac6e0cb12d45" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Remove the committed Context7 API key.
Line 7 stores a credential in the repository. Revoke and rotate this key before merge, purge it from Git history, and load it through the MCP client's supported secret mechanism instead.
🧰 Tools
🪛 Betterleaks (1.7.0)
[high] 7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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 @.mcp.json at line 7, Remove the hardcoded CONTEXT7_API_KEY value from
.mcp.json, revoke and rotate the exposed credential, purge it from Git history,
and configure the MCP client to obtain the key through its supported secret
mechanism instead.
Source: Linters/SAST tools
| defp extract_changeset_error_message(errors_list) do | ||
| errors_list | ||
| |> Enum.map(fn {field, {error, _}} -> "#{to_string(field)} #{error}" end) | ||
| |> List.foldr("", fn x, acc -> "#{acc <> x} " end) | ||
| |> Enum.map_join( | ||
| " ", | ||
| fn {field, messages} -> | ||
| formatted = | ||
| messages | ||
| |> List.wrap() | ||
| |> Enum.map_join(" ", &stringify_error_message/1) | ||
|
|
||
| "#{to_string(field)} #{formatted}" | ||
| end | ||
| ) | ||
| end | ||
|
|
||
| defp stringify_error_message(msg) when is_binary(msg), do: msg | ||
| defp stringify_error_message({msg, _opts}) when is_binary(msg), do: msg | ||
| defp stringify_error_message(%Ecto.Changeset{}), do: "embed invalid" | ||
| defp stringify_error_message(other), do: inspect(other) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how Library embed errors propagate into parent changesets.
rg -n 'cast_embed\(:library' lib/
rg -n 'traverse_errors' lib/ -A3 -B3
ast-grep run --pattern 'def changeset($_, $_) { $$$ }' --lang elixir lib/cadet/assessments/library.ex 2>/dev/null
cat -n lib/cadet/assessments/library.ex 2>/dev/null | head -80Repository: source-academy/backend
Length of output: 3822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Changeset definitions"
for f in lib/cadet/assessments/question.ex lib/cadet/assessments/assessment.ex; do
if [ -f "$f" ]; then
echo "--- $f line count ---"
wc -l "$f"
echo "--- changeset occurrences ---"
rg -n 'def changeset|cast_embed|errors|from_schema|validate_required|add_error' "$f" -A8 -B4
fi
done
echo "## XML parser relevant functions"
cat -n lib/cadet/jobs/xml_parser.ex | sed -n '1,70p;85,155p;330,375p'Repository: source-academy/backend
Length of output: 10994
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Embedded validation helpers"
for f in lib/cadet/assessments/question.ex lib/cadet/assessments/library.ex lib/cadet/assessments/external_library.ex; do
echo "--- $f line count ---"
wc -l "$f"
echo "--- relevant functions ---"
rg -n 'validate_arbitrary_embedded_struct_by_type|validate_programming_language_element|validate_conductor_no_external|validate_conductor_no_legacy_fields|to_changeset|changeset|cast_embed' "$f" -A12 -B4
done
echo "## Changeset insertion path"
if [ -f lib/cadet/assessments.ex ]; then
rg -n 'insert_or_update_assessments_and_questions|question_changeset|Question\.changeset|changeset\(.*Question|on_replace|cast_embed\(:library|cast_embed\(:grading_library' lib/cadet/assessments.ex -A12 -B4
fiRepository: source-academy/backend
Length of output: 6316
Traverse Library embed errors when logging changeset failures.
Question.changeset/2 casts :library and :grading_library as embeds, but Ecto.Changeset.errors/1 only reports top-level errors. Nested Library validation failures are not added as top-level {field, messages} tuples, so this path can log a generic embed error instead of the specific Library validation reason. Use traverse_errors/2 here, or have Library errors add a top-level error on the parent changeset.
🤖 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/xml_parser.ex` around lines 63 - 81, Update
extract_changeset_error_message to traverse nested embed errors via
Ecto.Changeset.traverse_errors/2, including validation messages from embedded
Library changesets for :library and :grading_library instead of reducing them to
the generic "embed invalid" text. Preserve the existing formatting and
stringification behavior for top-level errors.
| describe "Conductor programming language" do | ||
| test "happy path at TASK-level applies to all problems", %{ | ||
| course: course, | ||
| assessments_with_config: assessments_with_config | ||
| } do | ||
| conductor = | ||
| build(:programming_question, | ||
| library: build(:conductor_library), | ||
| grading_library: build(:conductor_library) | ||
| ) | ||
|
|
||
| for {assessment, assessment_config} <- assessments_with_config do | ||
| xml = XMLGenerator.generate_xml_for(assessment, [conductor]) | ||
| assert :ok == XMLParser.parse_xml(xml, course.id, assessment_config.id) | ||
| end | ||
| end | ||
|
|
||
| test "happy path per-PROBLEM override", %{ | ||
| course: course, | ||
| assessments_with_config: assessments_with_config | ||
| } do | ||
| conductor = | ||
| build(:programming_question, | ||
| library: build(:conductor_library), | ||
| grading_library: build(:conductor_library) | ||
| ) | ||
|
|
||
| for {assessment, assessment_config} <- assessments_with_config do | ||
| xml = XMLGenerator.generate_xml_for(assessment, [conductor]) | ||
| assert :ok == XMLParser.parse_xml(xml, course.id, assessment_config.id) | ||
| end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the task-level and override XML paths.
Line 292 emits the library from question.library. It does not use the generator library: or grading_library: options for TASK-level elements. Lines 291-310 therefore test the same per-PROBLEM path.
Create a TASK-level fixture through library: and grading_library:. Add a distinct per-PROBLEM library in the override test. Assert the persisted library values after parsing.
🤖 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 280 - 310, Update the
“Conductor programming language” tests to cover distinct XML paths: construct
the TASK-level fixture using the XML generator’s library: and grading_library:
options, and give the per-PROBLEM override fixture a different library. After
XMLParser.parse_xml, assert the persisted task-level and problem-level library
values so each path is verified independently.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (7)
.credo.exs (1)
97-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the global complexity limit at 11 unless a current violation requires 16.
Line 97 raises the limit for every analyzed module. The supplied
credo-original.txtandcredo-branch.txtreports contain noCyclomaticComplexityfinding. Run Credo with the limit set to 11 on this branch. If no violation appears, revert this change. Otherwise, refactor the specific function or document a scoped exception.Suggested default
- {Credo.Check.Refactor.CyclomaticComplexity, max_complexity: 16}, + {Credo.Check.Refactor.CyclomaticComplexity, max_complexity: 11},🤖 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 @.credo.exs at line 97, Revert the global max_complexity change in the Credo configuration to 11 unless running Credo at that limit reveals a current violation. If a violation exists, refactor the identified function or apply a narrowly scoped exception instead of raising the limit for all modules.test/support/xml_generator.ex (1)
225-233: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLegacy
library_attrsdropsvariantandexectime.
programminglanguage/2andgraderprogramminglanguage/2now permitvariantandexectimeattributes, butlibrary_attrs/1for the legacy (non-conductor) clause only returns%{interpreter: library.chapter}. LegacyLibrarystructs carryvariantandexec_time_ms. As written, no test built throughprocess_library/2can generate legacy XML withvariant/exectimeattributes, even though the tag now supports them.Add
variantandexectimeto the legacylibrary_attrs/1clause so tests can exercise variant/exectime round-tripping through the generator.♻️ Proposed fix
defp library_attrs(library) do - %{interpreter: library.chapter} + %{interpreter: library.chapter, variant: library[:variant], exectime: library[:exec_time_ms]} + |> Map.reject(fn {_k, v} -> is_nil(v) 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 `@test/support/xml_generator.ex` around lines 225 - 233, Update the legacy `library_attrs/1` clause to include the `variant` and `exectime` XML attributes, mapping them from the legacy `Library` fields `variant` and `exec_time_ms` while retaining the existing `interpreter: library.chapter` attribute.lib/cadet/jobs/xml_parser.ex (1)
296-331: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAllow mixed
library/grading_libraryformats if supported.
parse_programming_language/1can produce different:formatvalues for the two fields, and LambdaWorker routesquestion.grading_libraryby its own:format. Keep this parsing behavior if mixed legacy/conductor values are valid, or add an explicit validation rule that rejects it.🤖 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/xml_parser.ex` around lines 296 - 331, Update process_question_library/3 to explicitly handle mixed formats between the parsed library and grading_library values: preserve both results when mixed legacy/conductor formats are supported, or add validation that returns an error when their :format values differ. Ensure the behavior aligns with LambdaWorker routing grading_library by its own format.lib/cadet/jobs/autograder/result_store_worker.ex (1)
6-8: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider retries for the result-persistence queue.
max_attempts: 1discards the job after a single transient database error, so the autograding result is lost and the answer keepsautograding_status: :processing. The write path is idempotent peranswer_id, so a smallmax_attemptswith backoff is safe and recovers from short database outages.🤖 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 6 - 8, Update the Oban.Worker configuration for the result-persistence worker to use a small retry count instead of max_attempts: 1, preserving the idempotent answer_id write path and enabling Oban’s backoff to recover from transient database errors.test/cadet_web/controllers/stories_controller_test.exs (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the leftover
use Timex.This file no longer calls any Timex function after the migration. The
use Timexdirective at Line 3 is now dead setup and keeps this test coupled to Timex.🤖 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_web/controllers/stories_controller_test.exs` around lines 13 - 14, Remove the unused use Timex directive from the test module, leaving the DateTime-based setup and remaining test configuration unchanged.lib/cadet/jobs/autograder/lambda_worker.ex (1)
122-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
get_arg/3to a shared helper.
Cadet.Autograder.ResultStoreWorkerdefines the identical privateget_arg/3at lines 80-82 oflib/cadet/jobs/autograder/result_store_worker.ex. That module already importsCadet.SharedHelper. Put one implementation there and import it in both workers.🤖 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 122 - 124, Move the duplicated get_arg/3 implementation from the worker modules into Cadet.SharedHelper, then import or otherwise expose that shared helper in both Cadet.Autograder.LambdaWorker and Cadet.Autograder.ResultStoreWorker. Remove each worker’s private definition while preserving the existing key and default lookup behavior.lib/cadet/jobs/autograder/utilities.ex (1)
16-38: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnqueue the job in the same transaction as the status update.
Repo.update!/1commitsautograding_status: :processingbeforeOban.insert/1runs. If the insert fails, or the process stops between the two operations, the answer stays:processingand is never regraded, becausegrade_submission_question_answer_lists/5only regrades answers with status:noneor:failed. Wrap both operations in oneEcto.Multiand useOban.insert/3with the multi so the status change rolls back with a failed enqueue.🤖 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/utilities.ex` around lines 16 - 38, Update dispatch_programming_answer/3 to execute the Answer.autograding_changeset update and Oban enqueue within a single Ecto.Multi transaction, replacing the standalone Repo.update!/1 and Oban.insert/1 calls. Use Oban.insert/3 with the multi so any enqueue failure rolls back the :processing status update, while preserving the existing job arguments and overwrite behavior.
🤖 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 @.mcp.json:
- Line 7: Remove the hardcoded CONTEXT7_API_KEY value from .mcp.json, revoke and
rotate the exposed credential, purge it from Git history, and configure the MCP
client to obtain the key through its supported secret mechanism instead.
In `@lib/cadet_web/admin_controllers/admin_teams_controller.ex`:
- Around line 93-94: Update the course-scoped delete clause in
AdminTeamsController.delete to load the team through its assessment and validate
that assessment.course_id matches the requested course_id before deletion.
Return the existing 404/403 response when the team is missing or belongs to
another course, and only delegate to the team-id deletion path after this
authorization check.
In `@lib/cadet_web/controllers/generate_ai_comments.ex`:
- Line 53: Update check_llm_grading_parameters/3 to validate that the decrypted
LLM API key is non-empty before allowing the grading request to proceed. Return
the existing configuration-error result for an empty key, while preserving the
current behavior for valid keys and other parameter checks.
In `@lib/cadet_web/plug/cors.ex`:
- Around line 42-44: Update the CORS configuration lookup in the endpoint
settings flow to require an explicit :cors_endpoints value instead of defaulting
to "*". Reject "*" when credentialed CORS is enabled, while preserving valid
explicit origin-list handling and failing closed for missing or invalid
configuration.
In `@lib/cadet/jobs/autograder/lambda_worker.ex`:
- Around line 75-79: Update enqueue_result_store/1 to propagate the result of
Oban.insert/1 instead of discarding it, and make run_with_models/1 return that
result rather than unconditionally returning :ok. Apply the same result handling
to the enqueue_result_store/1 call in handle_failure/4 so insertion errors
propagate in both success and failure paths.
In `@lib/cadet/jobs/autograder/result_store_worker.ex`:
- Around line 67-78: Add fallback handling in normalize_status/1 for unknown
string or other status values, mapping them to a safe default status instead of
raising and leaving the answer in :processing; also add a non-map clause in
normalize_result/1 so nil results are normalized without crashing. Preserve the
existing mappings for "success", "failed", and atom statuses.
In `@lib/cadet/jobs/xml_parser.ex`:
- Around line 63-81: Update extract_changeset_error_message to traverse nested
embed errors via Ecto.Changeset.traverse_errors/2, including validation messages
from embedded Library changesets for :library and :grading_library instead of
reducing them to the generic "embed invalid" text. Preserve the existing
formatting and stringification behavior for top-level errors.
In `@test/cadet/updater/xml_parser_test.exs`:
- Around line 280-310: Update the “Conductor programming language” tests to
cover distinct XML paths: construct the TASK-level fixture using the XML
generator’s library: and grading_library: options, and give the per-PROBLEM
override fixture a different library. After XMLParser.parse_xml, assert the
persisted task-level and problem-level library values so each path is verified
independently.
---
Nitpick comments:
In @.credo.exs:
- Line 97: Revert the global max_complexity change in the Credo configuration to
11 unless running Credo at that limit reveals a current violation. If a
violation exists, refactor the identified function or apply a narrowly scoped
exception instead of raising the limit for all modules.
In `@lib/cadet/jobs/autograder/lambda_worker.ex`:
- Around line 122-124: Move the duplicated get_arg/3 implementation from the
worker modules into Cadet.SharedHelper, then import or otherwise expose that
shared helper in both Cadet.Autograder.LambdaWorker and
Cadet.Autograder.ResultStoreWorker. Remove each worker’s private definition
while preserving the existing key and default lookup behavior.
In `@lib/cadet/jobs/autograder/result_store_worker.ex`:
- Around line 6-8: Update the Oban.Worker configuration for the
result-persistence worker to use a small retry count instead of max_attempts: 1,
preserving the idempotent answer_id write path and enabling Oban’s backoff to
recover from transient database errors.
In `@lib/cadet/jobs/autograder/utilities.ex`:
- Around line 16-38: Update dispatch_programming_answer/3 to execute the
Answer.autograding_changeset update and Oban enqueue within a single Ecto.Multi
transaction, replacing the standalone Repo.update!/1 and Oban.insert/1 calls.
Use Oban.insert/3 with the multi so any enqueue failure rolls back the
:processing status update, while preserving the existing job arguments and
overwrite behavior.
In `@lib/cadet/jobs/xml_parser.ex`:
- Around line 296-331: Update process_question_library/3 to explicitly handle
mixed formats between the parsed library and grading_library values: preserve
both results when mixed legacy/conductor formats are supported, or add
validation that returns an error when their :format values differ. Ensure the
behavior aligns with LambdaWorker routing grading_library by its own format.
In `@test/cadet_web/controllers/stories_controller_test.exs`:
- Around line 13-14: Remove the unused use Timex directive from the test module,
leaving the DateTime-based setup and remaining test configuration unchanged.
In `@test/support/xml_generator.ex`:
- Around line 225-233: Update the legacy `library_attrs/1` clause to include the
`variant` and `exectime` XML attributes, mapping them from the legacy `Library`
fields `variant` and `exec_time_ms` while retaining the existing `interpreter:
library.chapter` attribute.
🪄 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 Plus
Run ID: 2a61d9f9-4b39-498d-bd1a-14441c0a266d
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.credo.exs.mcp.jsonconfig/config.exscredo-branch.txtcredo-original.txtlib/cadet/accounts/teams.exlib/cadet/application.exlib/cadet/assessments/assessment.exlib/cadet/assessments/assessments.exlib/cadet/assessments/library.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/assessments_helpers.exlib/cadet_web/helpers/view_helper.exlib/cadet_web/plug/cors.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/assessments/library_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/assessments/library_factory.extest/factories/stories/story_factory.extest/support/xml_generator.ex
💤 Files with no reviewable changes (1)
- lib/cadet/assessments/version_manager.ex
🛑 Comments failed to post (5)
lib/cadet_web/admin_controllers/admin_teams_controller.ex (1)
93-94: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline lib/cadet_web/admin_controllers/admin_teams_controller.ex --items all ast-grep outline lib/cadet/accounts/teams.ex --items all rg -n -C 5 'AdminTeamsController|/admin/teams|teamid|teamId' lib test rg -n -C 8 'def\s+delete_team\b|def\s+delete\b' \ lib/cadet/accounts/teams.ex \ lib/cadet_web/admin_controllers/admin_teams_controller.exRepository: source-academy/backend
Length of output: 37369
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== router admin stack ==" sed -n '240,292p' lib/cadet_web/router.ex | cat -n echo "== admin_teams_controller relevant section ==" sed -n '1,100p' lib/cadet_web/admin_controllers/admin_teams_controller.ex | cat -n echo "== admin_teams_controller_test delete section ==" sed -n '250,340p' test/cadet_web/admin_controllers/admin_teams_controller_test.exs | cat -n echo "== pipeline / authentication checks ==" fd -a '.*\.ex$' lib/cadet_web lib/cadet | xargs rg -n "pipe_through|authenticate|authorize|roles?|student_id|admin|Course|course_id|authorization" lib/cadet_web test/cadet_web -S | head -n 200Repository: source-academy/backend
Length of output: 33952
Ensure the deleted team belongs to the requested course.
This code discards
course_idand deletes anyTeamwith the suppliedteamId. A staff/admin user authorized for one course can delete a team from another course by supplying its ID. Load the team via its assessment and return 404/403 unlessteam.assessment.course_id == _course_id.🤖 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_teams_controller.ex` around lines 93 - 94, Update the course-scoped delete clause in AdminTeamsController.delete to load the team through its assessment and validate that assessment.course_id matches the requested course_id before deletion. Return the existing 404/403 response when the team is missing or belongs to another course, and only delegate to the team-id deletion path after this authorization check.lib/cadet_web/controllers/generate_ai_comments.ex (1)
53-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore validation for an empty LLM API key.
An API key that decrypts to
""now passescheck_llm_grading_parameters/3. The controller then callsOpenAI.chat_completion/2with an emptyapi_keyinstead of returning a configuration error. Validate the decrypted key before the LLM request.Proposed fix
-defp check_llm_grading_parameters(llm_model, llm_api_url, llm_course_level_prompt) do +defp check_llm_grading_parameters(llm_api_key, llm_model, llm_api_url, llm_course_level_prompt) do cond do + is_nil(llm_api_key) or llm_api_key == "" -> + {:parameter_error, "LLM API key is not configured for this course"} + is_nil(llm_model) or llm_model == "" -> {:parameter_error, "LLM model is not configured for this course"}check_llm_grading_parameters( + key, course.llm_model, course.llm_api_url, course.llm_course_level_prompt )Also applies to: 90-94
🤖 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` at line 53, Update check_llm_grading_parameters/3 to validate that the decrypted LLM API key is non-empty before allowing the grading request to proceed. Return the existing configuration-error result for an empty key, while preserving the current behavior for valid keys and other parameter checks.lib/cadet_web/plug/cors.ex (1)
42-44: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail fd --extension exs . config -x rg -n -C 2 'CadetWeb\.Endpoint|cors_endpoints' {}Repository: source-academy/backend
Length of output: 1287
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Files matching cors/ex config:" fd -i 'cors|corsica|endpoint' . | sed -n '1,120p' echo echo "Search cors_endpoints and corsica usage:" rg -n -C 4 'cors_endpoints|Corsica|corsica|allow_credentials' .Repository: source-academy/backend
Length of output: 7343
🌐 Web query:
hexdocs corsica 2.1 origins allow_credentials wildcard behavior💡 Result:
In the Corsica Elixir library (version 2.1), when
:originsis set to the wildcard"*"and:allow_credentialsis set totrue, the library automatically handles the conflict between the CORS specification and security requirements by dynamically echoing the origin [1][2]. Specifically, per the W3C CORS specification, theAccess-Control-Allow-Originheader cannot be"*"whenAccess-Control-Allow-Credentialsistrue[1][2]. To ensure compliance while still enabling the requested behavior, Corsica will set theAccess-Control-Allow-Originheader to the value of theOriginrequest header instead of the literal"*"[1][2]. This allows the request to succeed while strictly adhering to browser security policies [1][2].Citations:
- 1: https://corsica.hexdocs.pm/2.1.3/Corsica.html
- 2: https://hex.pm/packages/corsica/2.1.3/files/lib/corsica.ex
Require explicit CORS origins and reject wildcards when credentials are allowed.
The fallback at
lib/cadet_web/plug/cors.ex:44lets runtimes without:cors_endpointsuse Corsica’s wildcard origin handling withallow_credentials: true, so any request origin can receive credentialed responses. Fail closed or require an explicit origin list in production and reject"*".🤖 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/plug/cors.ex` around lines 42 - 44, Update the CORS configuration lookup in the endpoint settings flow to require an explicit :cors_endpoints value instead of defaulting to "*". Reject "*" when credentialed CORS is enabled, while preserving valid explicit origin-list handling and failing closed for missing or invalid configuration.lib/cadet/jobs/autograder/lambda_worker.ex (1)
75-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle the
Oban.insert/1result.
enqueue_result_store/1discards the return value, andrun_with_models/1then returns:ok. If the insert returns{:error, changeset}, the autograding result is silently dropped, the Oban job is recorded as completed, and the answer stays atautograding_status: :processingforever. The same applies to the call inhandle_failure/4at Line 91.🛠️ Proposed fix
defp enqueue_result_store(args) do - args - |> ResultStoreWorker.new() - |> Oban.insert() + case args |> ResultStoreWorker.new() |> Oban.insert() do + {:ok, job} -> + {:ok, job} + + {:error, reason} -> + message = + "Failed to enqueue autograder result. answer_id: #{get_arg(args, :answer_id)}, " <> + "reason: #{inspect(reason)}" + + Logger.error(message) + Sentry.capture_message(message) + {:error, message} + end endThen propagate the error from
run_with_models/1instead of returning:okunconditionally.🤖 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 75 - 79, Update enqueue_result_store/1 to propagate the result of Oban.insert/1 instead of discarding it, and make run_with_models/1 return that result rather than unconditionally returning :ok. Apply the same result handling to the enqueue_result_store/1 call in handle_failure/4 so insertion errors propagate in both success and failure paths.lib/cadet/jobs/autograder/result_store_worker.ex (1)
67-78: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a fallback clause for unknown statuses.
normalize_status/1matches only"success","failed", and atoms. Any other string raisesFunctionClauseErrorinsiderun/1. This worker has no rescue, andmax_attempts: 1discards the job, so the answer stays in:processingwith no stored result.normalize_result/1also crashes ifresultisnilbecause there is no non-map clause.🛡️ Proposed fix
defp normalize_result(result) when is_map(result) do %{ score: get_arg(result, :score), max_score: get_arg(result, :max_score), status: normalize_status(get_arg(result, :status)), result: get_arg(result, :result) } end defp normalize_status("success"), do: :success defp normalize_status("failed"), do: :failed defp normalize_status(status) when is_atom(status), do: status + defp normalize_status(status), do: raise(ArgumentError, "unknown autograding status: #{inspect(status)}")🤖 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 67 - 78, Add fallback handling in normalize_status/1 for unknown string or other status values, mapping them to a safe default status instead of raising and leaving the answer in :processing; also add a non-map clause in normalize_result/1 so nil results are normalized without crashing. Preserve the existing mappings for "success", "failed", and atom statuses.
3999d4b to
aa6105b
Compare
Conductor programming questions now build a grader request carrying
{format, language, evaluator} instead of raising, so they are autograded
by the external conductor runtime like legacy questions.
aa6105b to
0fcb3dd
Compare
2 participants
Adding support for conductor (external runtime) assessments with XOR constraint for backwards compatibility
This is part 2 of 3 in a stack made with GitButler: