Add beginner-friendly evaluation feedback for Python lessons - #180
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Some diagnostics disappear from the UI, and missing-function or near-match hints can be incorrectly classified.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds structured, beginner-friendly evaluation hints for Python lessons and displays them separately from technical diagnostics.
Changes:
- Infers lesson contracts and generates targeted learner hints.
- Propagates hint data through API and shared interfaces.
- Updates Outcome/Output rendering with focused tests.
File summaries
| File | Description |
|---|---|
codewit/lib/shared/interfaces/src/lib/output.ts |
Defines learner hint types. |
codewit/client/src/interfaces/evaluation.ts |
Adds hints to client responses. |
codewit/client/src/components/codeblock/CodeSubmission.tsx |
Separates hints from diagnostics. |
codewit/client/src/components/codeblock/CodeSubmission.spec.tsx |
Tests hint and output rendering. |
codewit/api/src/utils/learnerHints.ts |
Generates learner-facing hints. |
codewit/api/src/utils/learnerHints.spec.ts |
Tests hint generation scenarios. |
codewit/api/src/utils/exerciseContract.ts |
Infers lesson requirements. |
codewit/api/src/utils/exerciseContract.spec.ts |
Tests contract inference. |
codewit/api/src/models/attempt.ts |
Corrects optional creation fields. |
codewit/api/src/controllers/attempt.ts |
Adds hints to evaluation responses. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return [...new Set( | ||
| [...code.matchAll(/\b([A-Za-z_][A-Za-z0-9_]*)\b/g)] | ||
| .map((match) => match[1]) | ||
| .filter((identifier) => !reserved.has(identifier)) | ||
| )]; |
| const diagnosticText = buildDiagnosticText(detail); | ||
| const topicLabel = context.title?.trim() || context.topic?.trim() || 'This lesson'; | ||
| const lessonLabel = topicLabel; | ||
| const missingAttributeMatch = diagnosticText.match(/module 'program' has no attribute '([A-Za-z_][A-Za-z0-9_]*)'/); |
| const activeIssue = failure_details[issueIdx] || null; | ||
| const topLevelHint = 'learner_hint' in evaluation ? (evaluation.learner_hint ?? null) : null; | ||
| const activeHint = activeIssue?.learner_hint || topLevelHint || (state === 'passed' ? null : fallbackHint); | ||
| const technicalOutput = activeIssue?.rawout || compilation_error || runtime_error || error || ''; |
kbuffardi
left a comment
kbuffardi
left a comment
There was a problem hiding this comment.
The direction is valuable, but this needs changes before merge because the current implementation can hide diagnostics and confidently give incorrect guidance. The earlier inline findings remain unresolved, and the newly merged Codeval #25 parser exposes an additional cross-product correctness issue.
-
Multiple failures can be attributed to the wrong variable.
extractProgramAssertionIdentifierselects the last assertion found inrawout. Codeval's new parser attaches the complete pytest transcript to every failure detail, so with two failing tests an earlier issue can display the final test's variable name combined with the earlier test's expected/received values. Extract from the detail-specificerror_messagefirst, then fall back to a failure block scoped bytest_case. Please add an integration fixture with two failures and verify each generated hint names its own variable. -
Generated
from program import namefailures receive an unknown hint. The implementation only recognizes theAttributeErrorand custom-assertion forms, but Python reportsImportError: cannot import name 'name' from 'program'for the import style generated byseed-db.ts. Match that diagnostic and classify the imported identifier through the inferred contract, with tests for imported variables and functions. -
The Output tab can disappear while diagnostics exist.
technicalOutputignoresactiveIssue.error_messageandactiveIssue.stderr. This remains relevant outside Codeval's new pytest path; for example, Java failures can have an exception message but no captured output. Build the displayed output fromrawout,stderr, anderror_message, followed by top-level errors and explicit timeout/memory messages. Add coverage for each fallback. -
Near-match detection treats comments and strings as code. Scanning every identifier-like word can produce a high-confidence rename instruction based solely on a comment or string literal. Restrict candidates to actual definitions/bindings, or downgrade to a non-directive hint unless the name is structurally identified, and add regression tests.
-
Timeout and memory hints are not reachable through the current Codeval contract. Codeval initializes
execution_time_exceededandmemory_exceededto false and never sets them. Either update Codeval to emit these flags or classify its concrete runtime termination messages in Codewit before treating these specialized hints as supported.
Once these correctness and integration issues are addressed, this should be suitable for approval; the overall product direction is an improvement.
Use Codeval's per-failure diagnostics, recognize import-based missing names, and limit rename guidance to structurally identified Python bindings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Render failure and top-level diagnostic fallbacks, retain timeout streams, and reset issue navigation when a new evaluation arrives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Limit module export suggestions to top-level bindings, preserve every non-duplicate diagnostic, and keep timeout guidance ahead of partial test failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kbuffardi
left a comment
kbuffardi
left a comment
There was a problem hiding this comment.
All requested correctness and integration changes are addressed. Failure hints now consume Codeval's scoped diagnostics, import-based missing names and Python bindings are classified conservatively, timeout responses are structured and take precedence, and the UI preserves non-duplicate technical output while resetting issue navigation. The updated API and web checks pass.
Summary
referenceTest