feat(mcp-student): add student practice MCP#5090
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds a new apps/mcp-student FastMCP service (types, server, question-ref signing, GraphQL client, and service), integrates participant-scoped MCP JWTs and tutor-mode practice tools into apps/chat, introduces StudentPracticeQuizCard UI and practice submission API, updates shared components/types, adds Helm/Kubernetes templates and multi-arch CI workflows, and includes tests and documentation. ChangesStudent MCP Implementation
Chat App Integration
UI Components & Shared Elements
Deployment & CI
Tests & Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
|
dosubot
Bot
added
the
size:XXL
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new internal TypeScript FastMCP service (apps/mcp-student) to power student practice workflows from apps/chat via a participant-scoped GraphQL boundary, plus the associated shared types, chat UI integration, and Kubernetes/GitHub Actions deployment wiring.
Changes:
- Added
apps/mcp-student(FastMCP HTTP stream server) with signedquestionRefhandling, practice stack lookup/ranking, safe render payload shaping, and answer submission via persisted GraphQL ops. - Integrated chat with deterministic practice-candidate lookup, model-facing quiz-start tool, UI quiz card rendering (reusing shared renderer), and an authenticated submit API route.
- Added GraphQL query surface + persisted op for a participant-scoped course practice quiz, and added Helm + CI workflows to build/deploy the new service.
Reviewed changes
Copilot reviewed 53 out of 59 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Adds MCP-student env vars to Turborepo globalEnv for cache correctness. |
| project/STUDENT_MCP_CONCEPT.md | Documents the intended student MCP architecture, invariants, and rollout plan. |
| pnpm-lock.yaml | Adds lockfile entries for apps/mcp-student and related dependency changes. |
| packages/types/src/studentMcp.ts | Introduces shared student MCP contract/types (candidates, safe payload, errors). |
| packages/types/src/index.ts | Re-exports the student MCP types entrypoint. |
| packages/shared-components/src/StudentElement.tsx | Threads new compact prop through to question renderers. |
| packages/shared-components/src/NumericalQuestion.tsx | Adds compact layout mode to numerical question rendering. |
| packages/shared-components/src/FreeTextQuestion.tsx | Adds compact layout mode to free-text question rendering. |
| packages/shared-components/src/ChoicesQuestion.tsx | Adds compact layout mode to choices question rendering. |
| packages/graphql/src/services/courses.ts | Adds participant+chatbot-scoped getStudentMcpCoursePracticeQuiz wrapper. |
| packages/graphql/src/schema/query.ts | Exposes studentMcpCoursePracticeQuiz query for participants. |
| packages/graphql/src/public/schema.graphql | Publishes the new GraphQL query in the public schema artifact. |
| packages/graphql/src/public/client.json | Adds/updates persisted operation hashes used by clients. |
| packages/graphql/src/ops.schema.json | Updates ops schema JSON for the new query. |
| packages/graphql/src/graphql/ops/QGetCoursePracticeQuizWithoutSolutions.graphql | Adds persisted query used by MCP to fetch practice quiz without solutions. |
| packages/graphql/src/graphql/ops/FElementDataWithoutSolutions.graphql | Adjusts fragment fields used for answer-safe element payloads. |
| deploy/env-uzh-stg/values.yaml | Adds staging values for deploying mcpStudent. |
| deploy/env-uzh-prd/values.yaml | Adds production values for deploying mcpStudent. |
| deploy/charts/klicker-uzh-v3/values.yaml | Adds chart defaults for mcpStudent deployment/service/config. |
| deploy/charts/klicker-uzh-v3/templates/service-mcp-student.yaml | Adds ClusterIP Service template for mcp-student. |
| deploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yaml | Adds PodDisruptionBudget for mcp-student. |
| deploy/charts/klicker-uzh-v3/templates/hpa-mcp-student.yaml | Adds optional HPA template for mcp-student. |
| deploy/charts/klicker-uzh-v3/templates/deployment-mcp-student.yaml | Adds Deployment template for mcp-student including probes and env wiring. |
| deploy/charts/klicker-uzh-v3/templates/cm-mcp-student.yaml | Adds MCP-student ConfigMap (GraphQL endpoint, host/path/port, TTL). |
| deploy/charts/klicker-uzh-v3/templates/cm-chat.yaml | Injects MCP_STUDENT_URL into chat ConfigMap when enabled. |
| apps/mcp-student/vitest.config.ts | Adds Vitest config for the new service. |
| apps/mcp-student/tsconfig.json | Adds TypeScript config for dev/test. |
| apps/mcp-student/tsconfig.build.json | Adds build tsconfig for tsc emit. |
| apps/mcp-student/test/questionRef.test.ts | Adds unit tests for signing/verifying question refs. |
| apps/mcp-student/test/practice.test.ts | Adds unit tests for ranking, safety stripping, and submission validation. |
| apps/mcp-student/src/service.ts | Implements main service orchestration (lookup, fetch stack, submit answer). |
| apps/mcp-student/src/server.ts | Defines FastMCP server, tool schemas, auth, and error shaping. |
| apps/mcp-student/src/questionRef.ts | Implements signed questionRef codec (HMAC, expiry, context binding). |
| apps/mcp-student/src/practice.ts | Implements ranking, supported stack filtering, safe render payload mapping, and validation. |
| apps/mcp-student/src/index.ts | Wires config + GraphQL client + service + server startup. |
| apps/mcp-student/src/graphqlClient.ts | Implements persisted GraphQL client + operations used by MCP student. |
| apps/mcp-student/src/config.ts | Loads runtime env settings for the MCP service. |
| apps/mcp-student/src/auth.ts | Verifies participant JWT session for MCP calls. |
| apps/mcp-student/package.json | Adds package metadata/scripts/deps for @klicker-uzh/mcp-student. |
| apps/mcp-student/Dockerfile | Adds Docker build for the new MCP service. |
| apps/chat/test/student-practice-mcp.test.ts | Adds unit tests for the chat-side MCP adapter utilities. |
| apps/chat/test/mcpAuthMint.test.ts | Adds unit tests for minting/caching participant MCP JWTs. |
| apps/chat/src/services/studentPracticeMcp.ts | Adds chat-side MCP adapter (URL resolution, parsing, prompt formatting, tool calls). |
| apps/chat/src/services/mcpClients.ts | Extends MCP client creation to mint per-participant JWT for klicker-participant-jwt servers. |
| apps/chat/src/lib/server/mcpAuthMint.ts | Adds participant JWT minting + short-lived per-participant cache. |
| apps/chat/src/components/tool-fallback.tsx | Renders practice quiz tool result with a dedicated UI card. |
| apps/chat/src/components/student-practice-quiz-card.tsx | Adds chat UI practice quiz card (render, navigation, submit, archived handling). |
| apps/chat/src/app/layout.tsx | Wraps app with next-intl provider and sets <html lang> from locale. |
| apps/chat/src/app/api/chatbots/[chatbotId]/practice/submit/route.ts | Adds authenticated submit API endpoint for practice answers. |
| apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts | Integrates practice candidate lookup + model tool exposure + quiz rendering flow. |
| apps/chat/package.json | Adds @klicker-uzh/types dependency for shared MCP contract usage. |
| AGENTS.md | Documents MCP-student boundaries, deployment expectations, and chat integration constraints. |
| .github/workflows/v3_mcp-student-stg.yml | Adds CI workflow to build/push mcp-student images for staging. |
| .github/workflows/v3_mcp-student-prd.yml | Adds CI workflow to build/push mcp-student images for production tags. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| className={`${sourceSansPro.variable} ${monoSpaceFont.variable} font-sans antialiased`} | ||
| > | ||
| {children} | ||
| <NextIntlClientProvider>{children}</NextIntlClientProvider> |
Comment on lines
+31
to
+35
| const apiOrigin = (env.APP_ORIGIN_API ?? 'http://localhost:3000').replace( | ||
| /\/+$/, | ||
| '' | ||
| ) | ||
| const appSecret = env.APP_SECRET ?? 'abcd' |
| } | ||
|
|
||
| function orderedElements(stack: PracticeStack) { | ||
| return [...(stack.elements ?? [])].sort((a, b) => a.id - b.id) |
| name: element.elementData.name, | ||
| type: elementType, | ||
| content: element.elementData.content, | ||
| explanation: element.elementData.explanation ?? null, |
Comment on lines
+76
to
+81
| type: StudentMcpSupportedElementType | ||
| content: string | ||
| explanation?: string | null | ||
| basePoints?: boolean | ||
| pointsMultiplier?: number | ||
| options?: Record<string, unknown> | null |
Comment on lines
+165
to
+167
| responses.push({ | ||
| instanceId: element.id, | ||
| numericalResponse: parseFloat(value.response as string), |
Comment on lines
+47
to
+54
| const now = Date.now() | ||
| const cached = cache.get(participantId) | ||
| if (cached && now - cached.mintedAtMs <= CACHE_TTL_MS) { | ||
| return cached.jwt | ||
| } | ||
| if (cached) { | ||
| cache.delete(participantId) | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a036333a34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| function orderedElements(stack: PracticeStack) { | ||
| return [...(stack.elements ?? [])].sort((a, b) => a.id - b.id) |
There was a problem hiding this comment.
orderedElements re-sorts each stack by element.id, even though CourseService.getCoursePracticeQuiz already returns elements in the authored order sequence. This means students can see questions in a different order than configured (e.g., after instructors reorder a stack), and question-ref staleness checks will also ignore order-only changes because both payload creation and validation use this ID sort.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (12)
apps/mcp-student/src/graphqlClient.ts (2)
41-86: ⚡ Quick winAdd an explicit fetch timeout/AbortSignal.
executehas no per-request timeout. Backend GraphQL stalls will tie up the MCP tool until FastMCP's outertimeoutMs(10–30s) fires, which produces a less useful error and keeps the in-flight request open longer than necessary. A per-callAbortSignal.timeout(...)paired with a sensible default (e.g. 5–10s, configurable viaRuntimeSettings) gives clearer failure semantics and prevents request pile-up under degraded conditions.♻️ Sketch
async execute<TData, TVariables extends Record<string, unknown>>( operationName: string, variables: TVariables, - bearerToken: string + bearerToken: string, + signal?: AbortSignal ): Promise<TData> { const response = await this.fetchImpl(this.endpoint, { body: JSON.stringify({ ... }), headers: { ... }, method: 'POST', + signal: signal ?? AbortSignal.timeout(this.requestTimeoutMs), })🤖 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 `@apps/mcp-student/src/graphqlClient.ts` around lines 41 - 86, The execute method lacks a per-request timeout; update execute (in apps/mcp-student/src/graphqlClient.ts) to create an AbortSignal with a configurable timeout (via RuntimeSettings defaulting to ~5–10s) and pass that signal into this.fetchImpl call (alongside existing headers/body/method) so stalled GraphQL calls are aborted early; ensure you use AbortController/AbortSignal.timeout (or create a controller and setTimeout to call controller.abort()) and surface an informative error when the request is aborted, keeping the checks for response.ok, payload.errors, and payload.data unchanged.
104-130: ⚖️ Poor tradeoffGenerated GraphQL ↔
@klicker-uzh/typesshapes are bridged withas unknown as.The double-cast on lines 105 and 122–123 silently papers over any divergence between the generated
GetCoursePracticeQuizWithoutSolutionsQuery/RespondToElementStackMutationVariablesshapes and theStudent*mirrors in@klicker-uzh/types. If either shape drifts, type safety here won't catch it and runtime parsing will surface it instead.Consider adding a thin runtime guard (e.g. a Zod parse) at this boundary or aligning the types so this cast is unnecessary. Deferring to a follow-up is fine for chill scope, but worth tracking.
🤖 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 `@apps/mcp-student/src/graphqlClient.ts` around lines 104 - 130, The code is using unsafe double-casts (e.g., casting data.studentMcpCoursePracticeQuiz to PracticeQuiz and input.responses to RespondToElementStackMutationVariables['responses'] inside submitStackAnswer and the getter) which hides schema drift; replace the casts with a thin runtime guard at this GraphQL boundary: validate/parses the raw response and the outgoing responses (for example with Zod or a small type-check helper) before returning or passing into execute, or align the generated GraphQL types to the `@klicker-uzh/types` shapes so the cast is unnecessary; update the validation for data.studentMcpCoursePracticeQuiz, PracticeQuiz, and the input.responses used in submitStackAnswer (and use submitStackAnswer/execute as the location to perform this check).apps/mcp-student/src/practice.ts (1)
270-330: 💤 Low value
safeOptionsForElementsilently dropsFLASHCARDoptions.There's no branch for
FLASHCARD, so the finalreturn undefinedapplies. If that's intentional (flashcards have no client-renderable options), a one-line comment makes the intent explicit; otherwise FLASHCARD-specific fields will be missing from the safe payload without any indication.🤖 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 `@apps/mcp-student/src/practice.ts` around lines 270 - 330, The function safeOptionsForElement currently falls through to return undefined for elementType 'FLASHCARD', which silently drops any FLASHCARD options; update safeOptionsForElement to either handle 'FLASHCARD' explicitly (add a branch for elementType === 'FLASHCARD' that constructs and returns a sanitized safe object, using copyBooleanOption/copyStringOption/copyNumberOption as appropriate and validating any restrictions/fields) or add a one-line explanatory comment above the final return explaining that FLASHCARD intentionally has no client-renderable options (so returning undefined is expected); reference safeOptionsForElement and the elementType === 'FLASHCARD' branch when making the change.apps/mcp-student/src/server.ts (1)
74-115: ⚡ Quick winCouple
errorCodeto typed errors instead of message regex.
errorCoderecovers aStudentMcpToolErrorCodefrom the thrown error's message. This means any future tweak to the wording inpractice.ts('Submission must answer the complete stack','Duplicate response for instance ...', …) orquestionRef.tswill silently demote that error toUNKNOWN, and the chat client will lose its ability to branch on the specific failure. A small set of named error classes (or a single tagged error with acodefield) lets the throw site declare the code authoritatively, with this regex fallback retained only for unexpected errors.♻️ Sketch
// shared error type export class StudentPracticeError extends Error { constructor(public code: StudentMcpToolErrorCode, message: string) { super(message) } } // throw sites throw new StudentPracticeError('SUBMISSION_INVALID', 'Submission must answer the complete stack') // safeToolError function safeToolError(error: unknown): StudentMcpToolErrorOutput { if (error instanceof StudentPracticeError) { return { error: { code: error.code, message: error.message } } } // …existing regex fallback for legacy/unexpected errors → 'UNKNOWN' }🤖 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 `@apps/mcp-student/src/server.ts` around lines 74 - 115, Introduce a typed error class (e.g., StudentPracticeError extends Error with a public code: StudentMcpToolErrorCode) and update throw sites in practice.ts and questionRef.ts to throw that class with the proper code (e.g., 'SUBMISSION_INVALID', 'QUESTION_REF_EXPIRED', etc.); then modify safeToolError to first detect instance of StudentPracticeError and return { error: { code: error.code, message: error.message } } and only fall back to the existing errorCode(message) regex logic for unknown/legacy errors; update errorCode to remain as a legacy fallback but prefer the typed error path.apps/mcp-student/test/practice.test.ts (1)
110-135: 💤 Low valueSubstring assertions are fragile but acceptable for scope.
expect(serialized).not.toContain('correct' | 'feedback' | ...)will trip on legitimate content that happens to include those words. For now the fixture is engineered to avoid collisions, so this is fine — just keep in mind when extending the fixtures.🤖 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 `@apps/mcp-student/test/practice.test.ts` around lines 110 - 135, The current test uses fragile substring checks on serialized to assert sensitive fields are removed; replace those with structural checks: call toSafeStackRenderPayload(unsafe) then assert on the resulting object (safe or JSON.parse(serialized)) that the forbidden keys ('correct', 'feedback', 'exactSolutions', 'solutionRanges', 'sample answer') are not present on relevant nodes (e.g., inspect safe.elements[0].elementData and any nested options/choices) by checking for absence of those properties (e.g., using Object.prototype.hasOwnProperty or a utility like lodash.has) rather than using expect(serialized).not.toContain(...), and update the assertions around toSafeStackRenderPayload and safe.elements[0] accordingly.apps/mcp-student/src/index.ts (1)
14-26: ⚡ Quick winAdd graceful shutdown and top-level error handling for the production deployment.
This entrypoint runs in a containerized environment where the pod will see SIGTERM during rollouts and scale-downs. As written:
server.start(...)throwing rejects the top-level await with an unhandled rejection (no logging, exit code depends on Node).- There's no SIGTERM/SIGINT handler stopping the FastMCP HTTP listener, so in-flight tool calls during rollout get cut mid-request.
- No
process.on('unhandledRejection' | 'uncaughtException', ...)for last-resort logging.♻️ Suggested fix
-await server.start({ - httpStream: { - endpoint: settings.mcpEndpoint, - host: settings.host, - port: settings.port, - }, - transportType: 'httpStream', -}) - -console.log( - `Student MCP server listening on http://${settings.host}:${settings.port}${settings.mcpEndpoint}` -) +try { + await server.start({ + httpStream: { + endpoint: settings.mcpEndpoint, + host: settings.host, + port: settings.port, + }, + transportType: 'httpStream', + }) + console.log( + `Student MCP server listening on http://${settings.host}:${settings.port}${settings.mcpEndpoint}` + ) +} catch (error) { + console.error('Student MCP server failed to start', error) + process.exit(1) +} + +const shutdown = async (signal: string) => { + console.log(`Received ${signal}, stopping Student MCP server...`) + try { + await server.close() + } finally { + process.exit(0) + } +} + +process.on('SIGTERM', () => void shutdown('SIGTERM')) +process.on('SIGINT', () => void shutdown('SIGINT')) +process.on('unhandledRejection', (reason) => + console.error('Unhandled rejection in Student MCP', reason) +)🤖 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 `@apps/mcp-student/src/index.ts` around lines 14 - 26, Wrap the top-level await of server.start(...) in a try/catch to log any startup error and exit non‑zero, and register process-level handlers: add process.on('unhandledRejection', ...) and process.on('uncaughtException', ...) to log the error and exit; add SIGTERM and SIGINT handlers that call the MCP server shutdown method (e.g., server.stop() or server.shutdown() if defined) and await its completion before exiting so in-flight requests are drained; ensure logs include the error and that you use the existing server and settings symbols (server.start, server.stop/server.shutdown, settings.host/settings.port/settings.mcpEndpoint) when implementing these handlers.deploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yaml (1)
10-10:minAvailable: 1with the default single replica will block node drain.With
replicaCount: 1(the default invalues.yaml) and autoscaling disabled, the cluster cannot evict the single pod during node maintenance (kubectl drain) — it would violate this PDB. This blocks infrastructure upgrades silently.Consider switching to
maxUnavailable: 1(tolerates eviction while capping disruption) or making the PDB conditional onreplicaCount > 1.🔧 Option: use `maxUnavailable` instead
spec: - minAvailable: 1 + maxUnavailable: 1 selector:🤖 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 `@deploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yaml` at line 10, The PodDisruptionBudget currently sets minAvailable: 1 which will block drains when replicaCount is 1; update the template in pdb-mcp-student.yaml to either replace minAvailable: 1 with maxUnavailable: 1 (so a single replica can be evicted) or wrap the PDB block with a conditional that only renders it when .Values.replicaCount > 1; ensure you reference and use the Helm value .Values.replicaCount in the template logic so the PDB is not enforced for single-replica deployments.apps/chat/src/components/student-practice-quiz-card.tsx (1)
415-415: ⚡ Quick winConsider using disabled prop instead of pointer-events.
Using
pointer-events-noneprevents interaction but doesn't convey the disabled state to assistive technologies. Consider using thedisabledInputprop consistently and addingaria-disabledfor better accessibility.♿ Proposed accessibility enhancement
- {activeElement ? ( - <div className={isArchived ? 'pointer-events-none mt-4' : 'mt-4'}> + {activeElement ? ( + <div className="mt-4" aria-disabled={isArchived}> <StudentElementThe
disabledInputprop already handles the archived state on line 418.🤖 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 `@apps/chat/src/components/student-practice-quiz-card.tsx` at line 415, The div uses CSS pointer-events to block interaction but not expose disabled state to AT; update the component to rely on the existing disabledInput prop and add aria-disabled when archived: remove the use of 'pointer-events-none' on the wrapper controlled by isArchived, ensure interactive elements inside (e.g., buttons, inputs) receive disabled={disabledInput} and/or aria-disabled={isArchived} so the archived state is programmatically conveyed (reference isArchived and disabledInput in student-practice-quiz-card.tsx and apply aria-disabled on the wrapper and disabled on child inputs/buttons).apps/chat/src/services/studentPracticeMcp.ts (1)
9-9: ⚡ Quick winPrefer path alias for internal imports.
The import uses a relative path
./mcpClients. As per coding guidelines, use@or~path aliases for imports instead of relative paths.♻️ Suggested change
-import { createMCPClient, type MCPServerConfig } from './mcpClients' +import { createMCPClient, type MCPServerConfig } from '@/src/services/mcpClients'🤖 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 `@apps/chat/src/services/studentPracticeMcp.ts` at line 9, The import in studentPracticeMcp.ts uses a relative path; replace the relative import "import { createMCPClient, type MCPServerConfig } from './mcpClients'" with the project's path-alias form (e.g., "import { createMCPClient, type MCPServerConfig } from '@/services/mcpClients'" or the alias your tsconfig/webpack defines) so internal modules use the configured alias; update the import statement referencing createMCPClient and MCPServerConfig only, run type-check to ensure the alias resolves, and adjust any exports if needed.apps/chat/test/student-practice-mcp.test.ts (1)
2-9: ⚡ Quick winPrefer path aliases over relative imports.
The import uses a relative path
../src/services/studentPracticeMcp. As per coding guidelines, use@and~path aliases for imports instead of relative paths.♻️ Proposed fix
-import { - buildPracticeLookupContext, - formatPracticeCandidatesForPrompt, - getStudentPracticeMcpUrl, - parseMcpJsonToolResult, - StudentPracticeMcpToolError, - toPracticeCandidateId, -} from '../src/services/studentPracticeMcp' +import { + buildPracticeLookupContext, + formatPracticeCandidatesForPrompt, + getStudentPracticeMcpUrl, + parseMcpJsonToolResult, + StudentPracticeMcpToolError, + toPracticeCandidateId, +} from '@/src/services/studentPracticeMcp'🤖 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 `@apps/chat/test/student-practice-mcp.test.ts` around lines 2 - 9, The test file imports multiple symbols (buildPracticeLookupContext, formatPracticeCandidatesForPrompt, getStudentPracticeMcpUrl, parseMcpJsonToolResult, StudentPracticeMcpToolError, toPracticeCandidateId) via a relative path; replace the relative import from '../src/services/studentPracticeMcp' with the project path alias (using @ or ~ per codebase convention) so the import uses the alias form (e.g. '@/services/studentPracticeMcp' or '~/services/studentPracticeMcp'); ensure the alias is already configured in tsconfig/paths and your test runner config so the new import resolves.apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts (1)
1137-1137: 💤 Low valueConsider logging practice candidate count consistently.
The log includes
practiceCandidateCountwhich is good for observability. Consider adding it to other relevant log events for consistency across the practice flow.🤖 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 `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts at line 1137, The log currently includes practiceCandidateCount in one place; add the practiceCandidateCount field to all relevant practice-flow log events (e.g., the logs emitted from generatePracticeCandidates, startPracticeSession, completePracticeTurn, and candidate-selection/error paths) so the count is consistently present; update the logger calls (logger.info/logger.debug/logger.error or processLogger equivalents) to include practiceCandidateCount as a structured field alongside existing context (chatbotId, userId, stepId, etc.) and ensure each function that produces or handles candidates passes the practiceCandidateCount variable into its log call.apps/chat/src/services/mcpClients.ts (1)
7-7: 💤 Low valuePrefer path aliases for consistency.
The import uses a relative path
../lib/server/mcpAuthMint. For consistency with the codebase guidelines, consider using the@/path alias.♻️ Suggested change
-import { mintParticipantMcpJwt } from '../lib/server/mcpAuthMint' +import { mintParticipantMcpJwt } from '@/src/lib/server/mcpAuthMint'🤖 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 `@apps/chat/src/services/mcpClients.ts` at line 7, Update the import in mcpClients.ts to use the project path alias instead of a relative path: replace the relative import of mintParticipantMcpJwt from '../lib/server/mcpAuthMint' with the alias-based import (e.g., '@/lib/server/mcpAuthMint') so the symbol mintParticipantMcpJwt is imported via the `@/` path for consistency with the codebase.
🤖 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 @.github/workflows/v3_mcp-student-prd.yml:
- Line 17: The job conditions for build-arm and build-amd use
github.event.pull_request.draft directly and fail on push/tag events; update
each job's if to guard access by first checking the event name, e.g. replace
"if: github.event.pull_request.draft == false" with a short-circuiting check
like "if: github.event_name == 'pull_request' && github.event.pull_request.draft
== false" so the draft property is only accessed for pull_request events.
- Around line 24-41: The workflow is pinned to action releases that run on
Node16; update the action version pins to majors that target Node20+ (replace
actions/checkout@v3, docker/setup-buildx-action@v2, docker/metadata-action@v4
with their newer major releases that advertise Node20+ runtime), and make the
same replacements in the other similar block (the later occurrence referenced in
the comment). Ensure you update the three action uses named "actions/checkout",
"docker/setup-buildx-action", and "docker/metadata-action" so the workflow runs
on supported Node runtimes.
In @.github/workflows/v3_mcp-student-stg.yml:
- Line 30: The current `if: github.event.pull_request.draft == false` guard
skips jobs on push because `github.event.pull_request` is null; replace it with
a combined check that allows pushes and only filters pull requests by draft
status, for example use a condition like `(github.event_name == 'pull_request'
&& github.event.pull_request.draft == false) || (github.event_name == 'push')`
to fix the `if` for both occurrences (the existing `if` lines).
- Around line 37-54: Update the GitHub Actions steps that use older major
versions of Docker and checkout actions: replace actions/checkout@v3 with
actions/checkout@v4, docker/setup-qemu-action@v2 with
docker/setup-qemu-action@v3, docker/setup-buildx-action@v2 with
docker/setup-buildx-action@v3, docker/login-action@v2 with
docker/login-action@v3, and docker/metadata-action@v4 with
docker/metadata-action@v5; ensure you update every occurrence (including the
similar block referenced later) so the workflow uses the current major releases
and modern Node.js runtimes.
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/practice/submit/route.ts:
- Around line 78-81: The console.error call in route.ts is logging a sensitive
identifier (chatbotId); remove or mask the chatbotId from the log to avoid PII
leakage: update the failing-submit error path that currently logs { chatbotId,
error } to either omit chatbotId entirely or replace it with a non-identifying
token (e.g., maskedId or hashed value) and keep the error details only (refer to
the console.error invocation and the chatbotId variable in submit/route.ts);
ensure the error logging still provides context but does not include raw user
identifiers.
In `@apps/chat/src/components/tool-fallback.tsx`:
- Line 8: Import StudentPracticeQuizCard using the project's path alias instead
of a relative path; update the import statement in tool-fallback.tsx to
reference the module via the configured alias (e.g., replace
'./student-practice-quiz-card' with the appropriate '@' or '~' aliased path that
resolves to the StudentPracticeQuizCard component) so imports comply with the
path-alias guideline and tooling.
- Around line 75-77: Replace the hardcoded tool name string in the conditional
inside components/tool-fallback.tsx with the exported constant
STUDENT_PRACTICE_QUIZ_TOOL_NAME from '@/services/studentPracticeMcp': update the
if check that currently uses 'start_student_practice_quiz' to compare against
STUDENT_PRACTICE_QUIZ_TOOL_NAME, and add the corresponding import for
STUDENT_PRACTICE_QUIZ_TOOL_NAME near the top of the file so the branch rendering
StudentPracticeQuizCard (result/status) will use the canonical constant instead
of a magic string.
In `@apps/chat/src/lib/server/mcpAuthMint.ts`:
- Around line 47-67: The cache currently only removes an entry when the same
participantId is seen, causing unbounded growth; update the logic around cache
(the cache variable used in the function that reads participantId and
CACHE_TTL_MS) to prune stale entries opportunistically: before checking for
cached.get(participantId) iterate the cache keys and delete any entry where
Date.now() - entry.mintedAtMs > CACHE_TTL_MS (or perform a lightweight
sampling/limit to avoid long pauses for very large caches). Ensure you still
perform the existing per-participant TTL check (using now, cached.mintedAtMs,
CACHE_TTL_MS) and then set cache.set(participantId, { jwt, mintedAtMs: now }) as
before. This change touches the same function that calls signJWT and uses
variables participantId, cache, CACHE_TTL_MS, JWT_TTL_SECONDS, and mintedAtMs.
In `@apps/mcp-student/package.json`:
- Around line 32-34: Replace the exact Node engine comparator "=20" in the
package.json "engines" -> "node" field with a range allowing patch updates
(e.g., "20.x"); locate the "engines" object and update the "node" value from
"=20" to "20.x" so Voltla/pinned versions like 20.19.4 are accepted without
enforcing an exact 20.0.0 match.
In `@apps/mcp-student/src/config.ts`:
- Around line 35-45: Remove the insecure defaults and fail-fast in the config
construction: stop using 'abcd' for appSecret and require env.APP_SECRET and
env.APP_ORIGIN_AUTH (jwtIssuer) to be present; in the function that builds the
config (where appSecret, jwtIssuer and questionRefSecret are set) validate that
env.APP_SECRET is defined and throw a clear startup error if missing, validate
env.APP_ORIGIN_AUTH (jwtIssuer) and throw if missing, and set questionRefSecret
only from env.MCP_STUDENT_QUESTION_REF_SECRET or the validated appSecret (do not
tolerate a missing appSecret). Use the existing symbols appSecret, jwtIssuer and
questionRefSecret to locate and change the logic.
In `@apps/mcp-student/src/practice.ts`:
- Around line 84-98: The equality check in assertQuestionRefMatchesStack should
not rely on JSON.stringify ordering; replace the stringified comparison with a
structural comparison: build current via toQuestionRefPayload and then compare
ref.participantId, ref.chatbotId, ref.courseId against current's corresponding
fields and perform a deep structural equality check for the stack payload (or
call a known utility like lodash/isEqual) to ensure nested fields match; update
assertQuestionRefMatchesStack to throw the same error only when any of these
explicit field comparisons fail.
In `@deploy/charts/klicker-uzh-v3/templates/cm-chat.yaml`:
- Around line 22-24: The MCP_STUDENT_URL template can produce invalid URLs when
.Values.mcpStudent.path lacks a leading slash; update the template in
cm-chat.yaml where MCP_STUDENT_URL is built (the printf line using $fullName and
.Values.mcpStudent.path) to normalize the path by trimming any leading "/" and
prepending a single "/" (e.g., use trimPrefix "/" on .Values.mcpStudent.path and
then format "/%s"), or alternatively document/validate the requirement by adding
guidance to values.yaml / values.schema.json so callers must include the leading
slash.
In `@packages/types/src/studentMcp.ts`:
- Around line 71-82: StudentMcpSafeElementData currently permits leaking
solutions via the explanation and an untyped options bag; remove or make
explanation unavailable on the safe type and replace the loose options:
Record<string, unknown> | null with a new discriminated union
StudentMcpSafeElementOptions that enumerates only render-safe fields per
StudentMcpSupportedElementType (e.g., multipleChoice -> { choices: string[] },
numeric -> { min?: number; max?: number } or whatever render-only props you
support). Update StudentMcpSafeElementData to reference options?:
StudentMcpSafeElementOptions | null and ensure any code constructing safe
payloads only uses the new whitelisted option shapes and does not populate
explanation.
---
Nitpick comments:
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts:
- Line 1137: The log currently includes practiceCandidateCount in one place; add
the practiceCandidateCount field to all relevant practice-flow log events (e.g.,
the logs emitted from generatePracticeCandidates, startPracticeSession,
completePracticeTurn, and candidate-selection/error paths) so the count is
consistently present; update the logger calls
(logger.info/logger.debug/logger.error or processLogger equivalents) to include
practiceCandidateCount as a structured field alongside existing context
(chatbotId, userId, stepId, etc.) and ensure each function that produces or
handles candidates passes the practiceCandidateCount variable into its log call.
In `@apps/chat/src/components/student-practice-quiz-card.tsx`:
- Line 415: The div uses CSS pointer-events to block interaction but not expose
disabled state to AT; update the component to rely on the existing disabledInput
prop and add aria-disabled when archived: remove the use of
'pointer-events-none' on the wrapper controlled by isArchived, ensure
interactive elements inside (e.g., buttons, inputs) receive
disabled={disabledInput} and/or aria-disabled={isArchived} so the archived state
is programmatically conveyed (reference isArchived and disabledInput in
student-practice-quiz-card.tsx and apply aria-disabled on the wrapper and
disabled on child inputs/buttons).
In `@apps/chat/src/services/mcpClients.ts`:
- Line 7: Update the import in mcpClients.ts to use the project path alias
instead of a relative path: replace the relative import of mintParticipantMcpJwt
from '../lib/server/mcpAuthMint' with the alias-based import (e.g.,
'@/lib/server/mcpAuthMint') so the symbol mintParticipantMcpJwt is imported via
the `@/` path for consistency with the codebase.
In `@apps/chat/src/services/studentPracticeMcp.ts`:
- Line 9: The import in studentPracticeMcp.ts uses a relative path; replace the
relative import "import { createMCPClient, type MCPServerConfig } from
'./mcpClients'" with the project's path-alias form (e.g., "import {
createMCPClient, type MCPServerConfig } from '@/services/mcpClients'" or the
alias your tsconfig/webpack defines) so internal modules use the configured
alias; update the import statement referencing createMCPClient and
MCPServerConfig only, run type-check to ensure the alias resolves, and adjust
any exports if needed.
In `@apps/chat/test/student-practice-mcp.test.ts`:
- Around line 2-9: The test file imports multiple symbols
(buildPracticeLookupContext, formatPracticeCandidatesForPrompt,
getStudentPracticeMcpUrl, parseMcpJsonToolResult, StudentPracticeMcpToolError,
toPracticeCandidateId) via a relative path; replace the relative import from
'../src/services/studentPracticeMcp' with the project path alias (using @ or ~
per codebase convention) so the import uses the alias form (e.g.
'@/services/studentPracticeMcp' or '~/services/studentPracticeMcp'); ensure the
alias is already configured in tsconfig/paths and your test runner config so the
new import resolves.
In `@apps/mcp-student/src/graphqlClient.ts`:
- Around line 41-86: The execute method lacks a per-request timeout; update
execute (in apps/mcp-student/src/graphqlClient.ts) to create an AbortSignal with
a configurable timeout (via RuntimeSettings defaulting to ~5–10s) and pass that
signal into this.fetchImpl call (alongside existing headers/body/method) so
stalled GraphQL calls are aborted early; ensure you use
AbortController/AbortSignal.timeout (or create a controller and setTimeout to
call controller.abort()) and surface an informative error when the request is
aborted, keeping the checks for response.ok, payload.errors, and payload.data
unchanged.
- Around line 104-130: The code is using unsafe double-casts (e.g., casting
data.studentMcpCoursePracticeQuiz to PracticeQuiz and input.responses to
RespondToElementStackMutationVariables['responses'] inside submitStackAnswer and
the getter) which hides schema drift; replace the casts with a thin runtime
guard at this GraphQL boundary: validate/parses the raw response and the
outgoing responses (for example with Zod or a small type-check helper) before
returning or passing into execute, or align the generated GraphQL types to the
`@klicker-uzh/types` shapes so the cast is unnecessary; update the validation for
data.studentMcpCoursePracticeQuiz, PracticeQuiz, and the input.responses used in
submitStackAnswer (and use submitStackAnswer/execute as the location to perform
this check).
In `@apps/mcp-student/src/index.ts`:
- Around line 14-26: Wrap the top-level await of server.start(...) in a
try/catch to log any startup error and exit non‑zero, and register process-level
handlers: add process.on('unhandledRejection', ...) and
process.on('uncaughtException', ...) to log the error and exit; add SIGTERM and
SIGINT handlers that call the MCP server shutdown method (e.g., server.stop() or
server.shutdown() if defined) and await its completion before exiting so
in-flight requests are drained; ensure logs include the error and that you use
the existing server and settings symbols (server.start,
server.stop/server.shutdown, settings.host/settings.port/settings.mcpEndpoint)
when implementing these handlers.
In `@apps/mcp-student/src/practice.ts`:
- Around line 270-330: The function safeOptionsForElement currently falls
through to return undefined for elementType 'FLASHCARD', which silently drops
any FLASHCARD options; update safeOptionsForElement to either handle 'FLASHCARD'
explicitly (add a branch for elementType === 'FLASHCARD' that constructs and
returns a sanitized safe object, using
copyBooleanOption/copyStringOption/copyNumberOption as appropriate and
validating any restrictions/fields) or add a one-line explanatory comment above
the final return explaining that FLASHCARD intentionally has no
client-renderable options (so returning undefined is expected); reference
safeOptionsForElement and the elementType === 'FLASHCARD' branch when making the
change.
In `@apps/mcp-student/src/server.ts`:
- Around line 74-115: Introduce a typed error class (e.g., StudentPracticeError
extends Error with a public code: StudentMcpToolErrorCode) and update throw
sites in practice.ts and questionRef.ts to throw that class with the proper code
(e.g., 'SUBMISSION_INVALID', 'QUESTION_REF_EXPIRED', etc.); then modify
safeToolError to first detect instance of StudentPracticeError and return {
error: { code: error.code, message: error.message } } and only fall back to the
existing errorCode(message) regex logic for unknown/legacy errors; update
errorCode to remain as a legacy fallback but prefer the typed error path.
In `@apps/mcp-student/test/practice.test.ts`:
- Around line 110-135: The current test uses fragile substring checks on
serialized to assert sensitive fields are removed; replace those with structural
checks: call toSafeStackRenderPayload(unsafe) then assert on the resulting
object (safe or JSON.parse(serialized)) that the forbidden keys ('correct',
'feedback', 'exactSolutions', 'solutionRanges', 'sample answer') are not present
on relevant nodes (e.g., inspect safe.elements[0].elementData and any nested
options/choices) by checking for absence of those properties (e.g., using
Object.prototype.hasOwnProperty or a utility like lodash.has) rather than using
expect(serialized).not.toContain(...), and update the assertions around
toSafeStackRenderPayload and safe.elements[0] accordingly.
In `@deploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yaml`:
- Line 10: The PodDisruptionBudget currently sets minAvailable: 1 which will
block drains when replicaCount is 1; update the template in pdb-mcp-student.yaml
to either replace minAvailable: 1 with maxUnavailable: 1 (so a single replica
can be evicted) or wrap the PDB block with a conditional that only renders it
when .Values.replicaCount > 1; ensure you reference and use the Helm value
.Values.replicaCount in the template logic so the PDB is not enforced for
single-replica deployments.
🪄 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: ed08800f-ebd1-4c34-88b9-24a7a35442ff
⛔ Files ignored due to path filters (11)
packages/graphql/src/graphql/ops/FElementDataWithoutSolutions.graphqlis excluded by!**/**/graphql/ops/**packages/graphql/src/graphql/ops/QGetCoursePracticeQuizWithoutSolutions.graphqlis excluded by!**/**/graphql/ops/**packages/graphql/src/ops.schema.jsonis excluded by!**/**/ops.schema.jsonpackages/graphql/src/ops.tsis excluded by!**/**/ops.tspackages/graphql/src/public/client.jsonis excluded by!**/**/public/**packages/graphql/src/public/schema.graphqlis excluded by!**/**/public/**packages/graphql/src/public/server.jsonis excluded by!**/**/public/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlproject/screenshots/mcp-student-chat-after-submit.pngis excluded by!**/*.pngproject/screenshots/mcp-student-chat-answer-selected.pngis excluded by!**/*.pngproject/screenshots/mcp-student-chat-before-answer.pngis excluded by!**/*.png
📒 Files selected for processing (48)
.github/workflows/v3_mcp-student-prd.yml.github/workflows/v3_mcp-student-stg.ymlAGENTS.mdapps/chat/package.jsonapps/chat/src/app/api/chatbots/[chatbotId]/chat/route.tsapps/chat/src/app/api/chatbots/[chatbotId]/practice/submit/route.tsapps/chat/src/app/layout.tsxapps/chat/src/components/student-practice-quiz-card.tsxapps/chat/src/components/tool-fallback.tsxapps/chat/src/lib/server/mcpAuthMint.tsapps/chat/src/services/mcpClients.tsapps/chat/src/services/studentPracticeMcp.tsapps/chat/test/mcpAuthMint.test.tsapps/chat/test/student-practice-mcp.test.tsapps/mcp-student/Dockerfileapps/mcp-student/package.jsonapps/mcp-student/src/auth.tsapps/mcp-student/src/config.tsapps/mcp-student/src/graphqlClient.tsapps/mcp-student/src/index.tsapps/mcp-student/src/practice.tsapps/mcp-student/src/questionRef.tsapps/mcp-student/src/server.tsapps/mcp-student/src/service.tsapps/mcp-student/test/practice.test.tsapps/mcp-student/test/questionRef.test.tsapps/mcp-student/tsconfig.build.jsonapps/mcp-student/tsconfig.jsonapps/mcp-student/vitest.config.tsdeploy/charts/klicker-uzh-v3/templates/cm-chat.yamldeploy/charts/klicker-uzh-v3/templates/cm-mcp-student.yamldeploy/charts/klicker-uzh-v3/templates/deployment-mcp-student.yamldeploy/charts/klicker-uzh-v3/templates/hpa-mcp-student.yamldeploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yamldeploy/charts/klicker-uzh-v3/templates/service-mcp-student.yamldeploy/charts/klicker-uzh-v3/values.yamldeploy/env-uzh-prd/values.yamldeploy/env-uzh-stg/values.yamlpackages/graphql/src/schema/query.tspackages/graphql/src/services/courses.tspackages/shared-components/src/ChoicesQuestion.tsxpackages/shared-components/src/FreeTextQuestion.tsxpackages/shared-components/src/NumericalQuestion.tsxpackages/shared-components/src/StudentElement.tsxpackages/types/src/index.tspackages/types/src/studentMcp.tsproject/STUDENT_MCP_CONCEPT.mdturbo.json
|
|
||
| jobs: | ||
| build-arm: | ||
| if: github.event.pull_request.draft == false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the workflow file
cat -n .github/workflows/v3_mcp-student-prd.ymlRepository: uzh-bf/klicker-uzh
Length of output: 3313
🏁 Script executed:
# Also check if there are other workflow files to understand the pattern
git ls-files .github/workflows/Repository: uzh-bf/klicker-uzh
Length of output: 1857
🌐 Web query:
GitHub Actions github.event.pull_request undefined on push events behavior
💡 Result:
On push events, github.event.pull_request is undefined (because github.event is the event payload for the specific event that triggered the workflow, and push payloads do not include a pull_request object) [1][2]. What this means in practice 1) If your workflow is triggered by on: push, you cannot rely on github.event.pull_request.; those properties won’t exist for that run. - GitHub states that github.event is the full webhook payload of the triggering event, and that its contents differ by event type [1]. - The documented webhook payload for push is the PushEvent payload, which includes fields like ref, head, and before, but not pull_request [2]. 2) If you use pull_request. fields inside conditions, guard them using github.event_name (the event type), not github.event. - GitHub recommends using github.event_name for deciding whether the run was triggered by pull_request/push, rather than comparing github.event to a string. An Actions runner issue notes that if: github.event == 'push' is incorrect, and should be if: github.event_name == 'push' [3]. 3) If you want pull_request data, trigger (or also trigger) the workflow with pull_request (or pull_request_target). - Workflows triggered by pull_request run with the pull_request webhook payload (so github.event.pull_request.* is available) [4]. Example pattern - Run steps only for pull requests: if: ${{ github.event_name == 'pull_request' }} - Then you can safely reference github.event.pull_request.* inside that guarded step. If instead you triggered on both push and pull_request, then on push runs github.event.pull_request will still be undefined, and your guard must prevent evaluating pull_request.* on push runs.
Citations:
- 1: https://docs.github.com/en/actions/reference/contexts-reference
- 2: https://docs.github.com/en/actions/reference/events-that-trigger-workflows
- 3: Docs:
if: github.event == 'push'doesn't work, it should begithub.event_nameactions/runner#950 - 4: https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/triggering-a-workflow
Tag builds will be skipped due to undefined pull_request property on push events.
Lines 17 and 57 evaluate github.event.pull_request.draft == false on push tag events where github.event.pull_request is undefined. This causes the condition to fail and prevents the jobs from running. The concurrency group at line 8 already guards properly with a fallback operator, but the job conditions must be updated.
Use github.event_name to guard access to pull_request properties:
🔧 Proposed fix
- if: github.event.pull_request.draft == false
+ if: github.event_name != 'pull_request' || github.event.pull_request.draft == falseApply to both line 17 (build-arm) and line 57 (build-amd).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if: github.event.pull_request.draft == false | |
| if: github.event_name != 'pull_request' || github.event.pull_request.draft == false |
🤖 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 @.github/workflows/v3_mcp-student-prd.yml at line 17, The job conditions for
build-arm and build-amd use github.event.pull_request.draft directly and fail on
push/tag events; update each job's if to guard access by first checking the
event name, e.g. replace "if: github.event.pull_request.draft == false" with a
short-circuiting check like "if: github.event_name == 'pull_request' &&
github.event.pull_request.draft == false" so the draft property is only accessed
for pull_request events.
klicker-uzh
 |
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
codex/mcp-student-v3-ai
|
| Run status |
|
| Run duration | 06m 33s |
| Commit |
|
| Committer | Roland Schläfli |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
1
|
 |
0
|
 |
0
|
 |
758
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/mcp-student/test/graphqlClient.test.ts (2)
27-29: ⚡ Quick winAssert the exact persisted hash, not just hash shape
The regex check only validates format. This test will still pass if a wrong 64-char hash is sent, which can break persisted-query execution at runtime. Prefer asserting the expected hash for
GetCoursePracticeQuizWithoutSolutions(from generated artifacts) to make this regression-proof.🤖 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 `@apps/mcp-student/test/graphqlClient.test.ts` around lines 27 - 29, The test currently only validates the persisted query hash shape; replace the regex assertion with an exact equality check using the known SHA256 for the GetCoursePracticeQuizWithoutSolutions persisted query: locate the expected hash in your generated artifacts (the persisted queries map) and change the assertion on body.extensions.persistedQuery.sha256Hash to expect(...).toBe('<expected_hash_here>') while keeping the existing operationName check (body.operationName === 'GetCoursePracticeQuizWithoutSolutions') so the test verifies the exact persisted hash rather than just its format.
2-2: ⚡ Quick winUse repo path aliases instead of a relative import
Please switch
../src/graphqlClient.jsto the configured@/~alias style for consistency with project import resolution rules.As per coding guidelines: "
**/*.{ts,tsx,js,jsx}: Use@and~path aliases for imports".🤖 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 `@apps/mcp-student/test/graphqlClient.test.ts` at line 2, Change the relative import of PersistedGraphQLClient to use the project's path alias instead of ../src/graphqlClient.js; locate the import line that references PersistedGraphQLClient and replace the path with the configured alias (for example import { PersistedGraphQLClient } from "@/graphqlClient" or "@/graphqlClient.js" depending on build settings) so it follows the project's `@/`~ alias convention and resolves via the repo's module alias config.deploy/charts/klicker-uzh-v3/templates/deployment-mcp-student.yaml (1)
43-48: ⚡ Quick winConsider enabling read-only root filesystem for security hardening.
The container security context is templated from values, which provides flexibility. However, for defense-in-depth, consider setting
readOnlyRootFilesystem: truein the default values to prevent runtime modifications to the container filesystem. The mcp-student app should write to mounted volumes or emptyDir if temporary storage is needed.🛡️ Recommended security context in values
In your values file, ensure the mcp-student security context includes:
mcpStudent: securityContext: readOnlyRootFilesystem: true allowPrivilegeEscalation: false runAsNonRoot: true capabilities: drop: - ALLIf the app requires writable directories (e.g., for temp files), mount an emptyDir volume.
🤖 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 `@deploy/charts/klicker-uzh-v3/templates/deployment-mcp-student.yaml` around lines 43 - 48, Add a default read-only root filesystem and other hardening defaults to the mcp-student container securityContext in the Helm values so the rendered deployment sets readOnlyRootFilesystem: true (and related settings) by default; update the mcpStudent.values block to include readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, runAsNonRoot: true and drop capabilities (so the template that renders .Values.mcpStudent.securityContext for container name "mcp-student" picks them up), and if the app needs writeable paths, add an emptyDir-mounted volume in the same values/template so writable dirs are explicit instead of the root filesystem.deploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yaml (1)
10-10: ⚡ Quick winPDB maxUnavailable may block disruptions with single replica.
When
mcpStudent.replicaCountis 1 (common in non-production environments),maxUnavailable: 1prevents voluntary disruptions (node drains, rolling updates), potentially blocking cluster maintenance. Consider either:
- Making
maxUnavailableconfigurable via values- Using
minAvailablebased on replica count- Conditionally creating the PDB only when
replicaCount > 1♻️ Proposed fix: Make PDB conditional on replica count
-{{- if .Values.mcpStudent.enabled -}} +{{- if and .Values.mcpStudent.enabled (gt (.Values.mcpStudent.replicaCount | int) 1) -}} apiVersion: policy/v1 kind: PodDisruptionBudgetOr make maxUnavailable configurable:
spec: - maxUnavailable: 1 + maxUnavailable: {{ .Values.mcpStudent.pdb.maxUnavailable | default 1 }}🤖 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 `@deploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yaml` at line 10, PDB with maxUnavailable: 1 can block disruptions when mcpStudent.replicaCount == 1; update the pdb-mcp-student.yaml Helm template to avoid that by either (A) making the PDB conditional—only render the PodDisruptionBudget resource when .Values.mcpStudent.replicaCount > 1 (wrap creation with a replica-count check), or (B) make the setting configurable by adding .Values.mcpStudent.pdb.maxUnavailable (or .pdb.minAvailable) in values.yaml and use that value in the template so operators can set safe defaults for single-replica environments; target the PodDisruptionBudget resource and the maxUnavailable field in the template when applying the change.
🤖 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 `@apps/chat/package.json`:
- Around line 58-59: The CI is failing with ERR_PNPM_OUTDATED_LOCKFILE because
the new dependencies "@tailwindcss/aspect-ratio" and "@tailwindcss/forms" were
added to apps/chat/package.json but pnpm-lock.yaml wasn’t updated; run pnpm
install from the repository root to regenerate/update pnpm-lock.yaml so it
includes those packages (ensure the lockfile is committed afterwards).
In `@deploy/charts/klicker-uzh-v3/templates/deployment-mcp-student.yaml`:
- Around line 56-60: The chart references a runtime secret for APP_SECRET via
the mcpStudent values (mcpStudent.secret.name / default printf "%s-secret-chat"
$fullName and mcpStudent.secret.appSecretKey) but the chart no longer creates
that secret; add back a template that creates the secret (e.g.,
secret-chat.yaml) which reads the value from values or generates a random value
when not provided, or alternatively wire in an external-secret resource (e.g.,
ExternalSecret or SealedSecret) and document the required key name (APP_SECRET)
and values.mcpStudent.secret.* fields; ensure the template/resource uses the
same name resolution as the deployment (the default printf "%s-secret-chat"
$fullName) and populates the key matching mcpStudent.secret.appSecretKey so the
APP_SECRET env var can be resolved at pod startup.
---
Nitpick comments:
In `@apps/mcp-student/test/graphqlClient.test.ts`:
- Around line 27-29: The test currently only validates the persisted query hash
shape; replace the regex assertion with an exact equality check using the known
SHA256 for the GetCoursePracticeQuizWithoutSolutions persisted query: locate the
expected hash in your generated artifacts (the persisted queries map) and change
the assertion on body.extensions.persistedQuery.sha256Hash to
expect(...).toBe('<expected_hash_here>') while keeping the existing
operationName check (body.operationName ===
'GetCoursePracticeQuizWithoutSolutions') so the test verifies the exact
persisted hash rather than just its format.
- Line 2: Change the relative import of PersistedGraphQLClient to use the
project's path alias instead of ../src/graphqlClient.js; locate the import line
that references PersistedGraphQLClient and replace the path with the configured
alias (for example import { PersistedGraphQLClient } from "@/graphqlClient" or
"@/graphqlClient.js" depending on build settings) so it follows the project's
`@/`~ alias convention and resolves via the repo's module alias config.
In `@deploy/charts/klicker-uzh-v3/templates/deployment-mcp-student.yaml`:
- Around line 43-48: Add a default read-only root filesystem and other hardening
defaults to the mcp-student container securityContext in the Helm values so the
rendered deployment sets readOnlyRootFilesystem: true (and related settings) by
default; update the mcpStudent.values block to include readOnlyRootFilesystem:
true, allowPrivilegeEscalation: false, runAsNonRoot: true and drop capabilities
(so the template that renders .Values.mcpStudent.securityContext for container
name "mcp-student" picks them up), and if the app needs writeable paths, add an
emptyDir-mounted volume in the same values/template so writable dirs are
explicit instead of the root filesystem.
In `@deploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yaml`:
- Line 10: PDB with maxUnavailable: 1 can block disruptions when
mcpStudent.replicaCount == 1; update the pdb-mcp-student.yaml Helm template to
avoid that by either (A) making the PDB conditional—only render the
PodDisruptionBudget resource when .Values.mcpStudent.replicaCount > 1 (wrap
creation with a replica-count check), or (B) make the setting configurable by
adding .Values.mcpStudent.pdb.maxUnavailable (or .pdb.minAvailable) in
values.yaml and use that value in the template so operators can set safe
defaults for single-replica environments; target the PodDisruptionBudget
resource and the maxUnavailable field in the template when applying the change.
🪄 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: aa7af9f9-fc61-44ea-acb0-bb4d11d2777b
⛔ Files ignored due to path filters (5)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlproject/screenshots/mcp-student-chat-after-submit.pngis excluded by!**/*.pngproject/screenshots/mcp-student-chat-answer-selected.pngis excluded by!**/*.pngproject/screenshots/mcp-student-chat-archived-history.pngis excluded by!**/*.pngproject/screenshots/mcp-student-chat-before-answer.pngis excluded by!**/*.png
📒 Files selected for processing (19)
.github/workflows/v3_mcp-student-prd.yml.github/workflows/v3_mcp-student-stg.ymlapps/chat/package.jsonapps/chat/src/app/api/chatbots/[chatbotId]/practice/submit/route.tsapps/chat/src/app/layout.tsxapps/chat/src/components/student-practice-quiz-card.tsxapps/chat/src/components/tool-fallback.tsxapps/chat/src/lib/server/mcpAuthMint.tsapps/mcp-student/package.jsonapps/mcp-student/src/config.tsapps/mcp-student/src/graphqlClient.tsapps/mcp-student/src/practice.tsapps/mcp-student/src/server.tsapps/mcp-student/test/graphqlClient.test.tsapps/mcp-student/test/practice.test.tsdeploy/charts/klicker-uzh-v3/templates/cm-chat.yamldeploy/charts/klicker-uzh-v3/templates/deployment-mcp-student.yamldeploy/charts/klicker-uzh-v3/templates/pdb-mcp-student.yamlpackages/types/src/studentMcp.ts
✅ Files skipped from review due to trivial changes (1)
- apps/mcp-student/package.json
🚧 Files skipped from review as they are similar to previous changes (14)
- deploy/charts/klicker-uzh-v3/templates/cm-chat.yaml
- apps/mcp-student/test/practice.test.ts
- apps/chat/src/components/tool-fallback.tsx
- apps/mcp-student/src/config.ts
- .github/workflows/v3_mcp-student-stg.yml
- apps/chat/src/lib/server/mcpAuthMint.ts
- apps/chat/src/app/api/chatbots/[chatbotId]/practice/submit/route.ts
- packages/types/src/studentMcp.ts
- apps/mcp-student/src/graphqlClient.ts
- .github/workflows/v3_mcp-student-prd.yml
- apps/mcp-student/src/server.ts
- apps/chat/src/components/student-practice-quiz-card.tsx
- apps/chat/src/app/layout.tsx
- apps/mcp-student/src/practice.ts
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Member
Author
|
Pushed follow-up commit 5798140 ( |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/mcp-student/src/graphqlClient.ts`:
- Around line 59-75: The HttpLink created in the constructor for ApolloClient
uses fetchImpl without an abort timeout, so wrap fetchImpl with an abort-capable
wrapper using AbortController and pass its signal into the request; add an
optional timeoutMs parameter to the constructor (default e.g. 30_000) and start
a setTimeout that calls controller.abort() after timeoutMs, ensuring you
clearTimeout on response; update the HttpLink instantiation in the constructor
to use this wrapped fetch function (still named fetchImpl externally) so
outgoing GraphQL fetches created by ApolloClient/HttpLink honor the timeout and
abort properly.
- Around line 171-173: Remove the unsafe "as unknown as" casts and fix the
incorrect Promise/unknown return at the GraphQL contract boundary: use the types
produced by the generated GraphQL types rather than bridging through unknown.
Specifically, in the function returning data.studentMcpCoursePracticeQuiz (typed
by GetCoursePracticeQuizWithoutSolutionsQuery) remove the `as unknown as
PracticeQuiz` cast and return the value directly; change the return type of the
responder function from `Promise<unknown>` to
`RespondToElementStackMutation['respondToElementStack'] | null`; and remove the
`as unknown` cast on `input.responses` so you pass the generated mutation
variable type directly to the mutation call (no unknown bridging). Ensure
signatures and return annotations reference
GetCoursePracticeQuizWithoutSolutionsQuery and RespondToElementStackMutation
types accordingly.
🪄 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: e8516e9f-a718-4233-bbf2-242b5f3c12a4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
AGENTS.mdapps/mcp-student/package.jsonapps/mcp-student/src/graphqlClient.ts
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/mcp-student/package.json
Comment on lines
+59
to
+75
| constructor(endpoint: string, fetchImpl: typeof fetch = fetch) { | ||
| this.client = new ApolloClient({ | ||
| cache: new InMemoryCache(), | ||
| link: from([ | ||
| createPersistedQueryLink({ | ||
| generateHash: (document) => | ||
| operationHash(documentOperationName(document)), | ||
| retry: () => false, | ||
| }), | ||
| new HttpLink({ | ||
| fetch: fetchImpl, | ||
| headers: { | ||
| Accept: 'application/json', | ||
| 'x-graphql-yoga-csrf': 'true', | ||
| }, | ||
| uri: endpoint, | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that graphqlClient currently has HttpLink but no timeout/abort handling.
rg -n --type ts "new HttpLink|AbortController|timeout|signal" apps/mcp-student/src/graphqlClient.tsRepository: uzh-bf/klicker-uzh
Length of output: 88
🏁 Script executed:
cat -n apps/mcp-student/src/graphqlClient.tsRepository: uzh-bf/klicker-uzh
Length of output: 6825
Add a timeout to outbound GraphQL fetches.
HttpLink currently uses fetch without an abort timeout, so a slow/hung backend call can block MCP request handling indefinitely.
Proposed fix
+const GRAPHQL_TIMEOUT_MS = 10_000
+
+function withTimeout(
+ fetchImpl: typeof fetch,
+ timeoutMs: number
+): typeof fetch {
+ return async (input, init?: RequestInit) => {
+ const controller = new AbortController()
+ const timeout = setTimeout(() => controller.abort(), timeoutMs)
+ try {
+ return await fetchImpl(input, {
+ ...init,
+ signal: controller.signal,
+ })
+ } finally {
+ clearTimeout(timeout)
+ }
+ }
+}
+
export class PersistedGraphQLClient {
private readonly client: ApolloClient<NormalizedCacheObject>
constructor(endpoint: string, fetchImpl: typeof fetch = fetch) {
+ const fetchWithTimeout = withTimeout(fetchImpl, GRAPHQL_TIMEOUT_MS)
this.client = new ApolloClient({
cache: new InMemoryCache(),
link: from([
createPersistedQueryLink({
generateHash: (document) =>
operationHash(documentOperationName(document)),
retry: () => false,
}),
new HttpLink({
- fetch: fetchImpl,
+ fetch: fetchWithTimeout,
headers: {
Accept: 'application/json',
'x-graphql-yoga-csrf': 'true',
},
uri: endpoint,
}),
]),
})
}🤖 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 `@apps/mcp-student/src/graphqlClient.ts` around lines 59 - 75, The HttpLink
created in the constructor for ApolloClient uses fetchImpl without an abort
timeout, so wrap fetchImpl with an abort-capable wrapper using AbortController
and pass its signal into the request; add an optional timeoutMs parameter to the
constructor (default e.g. 30_000) and start a setTimeout that calls
controller.abort() after timeoutMs, ensuring you clearTimeout on response;
update the HttpLink instantiation in the constructor to use this wrapped fetch
function (still named fetchImpl externally) so outgoing GraphQL fetches created
by ApolloClient/HttpLink honor the timeout and abort properly.
Comment on lines
+171
to
+173
| return ( | ||
| (data.studentMcpCoursePracticeQuiz as unknown as PracticeQuiz | null) ?? | ||
| null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current unsafe bridge casts and unknown-return usage in this file.
rg -n --type ts "as unknown as|Promise<unknown>" apps/mcp-student/src/graphqlClient.tsRepository: uzh-bf/klicker-uzh
Length of output: 270
🏁 Script executed:
cat -n apps/mcp-student/src/graphqlClient.ts | head -200Repository: uzh-bf/klicker-uzh
Length of output: 6825
🏁 Script executed:
# Check what GraphQL types are imported
rg -n "import.*from.*@klicker-uzh/graphql" apps/mcp-student/src/graphqlClient.ts | head -20Repository: uzh-bf/klicker-uzh
Length of output: 124
🏁 Script executed:
# Verify the types mentioned in the proposed fix exist
rg -n "PracticeQuiz|RespondToElementStackMutation" apps/mcp-student/src/graphqlClient.tsRepository: uzh-bf/klicker-uzh
Length of output: 885
Remove unsafe unknown bridge casts and fix return type at GraphQL contract boundary.
Three instances of as unknown as and Promise<unknown> hide type information and violate strict-mode safety:
- Line 172:
data.studentMcpCoursePracticeQuizis already properly typed byGetCoursePracticeQuizWithoutSolutionsQuery—the cast is unnecessary. - Line 180: Return type should be
RespondToElementStackMutation['respondToElementStack'] | nullinstead ofPromise<unknown>. - Line 190:
input.responsestype already matches the mutation variables; remove the unsafe cast.
Proposed fix
export type SubmitStackAnswerInput = {
courseId: string
- responses: StackResponseInput[]
+ responses: RespondToElementStackMutationVariables['responses']
stackAnswerTimeSeconds: number
stackId: number
}
@@
async getCoursePracticeQuiz(
@@
- return (
- (data.studentMcpCoursePracticeQuiz as unknown as PracticeQuiz | null) ??
- null
- )
+ return data.studentMcpCoursePracticeQuiz ?? null
}
@@
async submitStackAnswer(
input: SubmitStackAnswerInput,
bearerToken: string
- ): Promise<unknown> {
+ ): Promise<RespondToElementStackMutation['respondToElementStack'] | null> {
@@
- responses:
- input.responses as unknown as RespondToElementStackMutationVariables['responses'],
+ responses: input.responses,🤖 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 `@apps/mcp-student/src/graphqlClient.ts` around lines 171 - 173, Remove the
unsafe "as unknown as" casts and fix the incorrect Promise/unknown return at the
GraphQL contract boundary: use the types produced by the generated GraphQL types
rather than bridging through unknown. Specifically, in the function returning
data.studentMcpCoursePracticeQuiz (typed by
GetCoursePracticeQuizWithoutSolutionsQuery) remove the `as unknown as
PracticeQuiz` cast and return the value directly; change the return type of the
responder function from `Promise<unknown>` to
`RespondToElementStackMutation['respondToElementStack'] | null`; and remove the
`as unknown` cast on `input.responses` so you pass the generated mutation
variable type directly to the mutation call (no unknown bridging). Ensure
signatures and return annotations reference
GetCoursePracticeQuizWithoutSolutionsQuery and RespondToElementStackMutation
types accordingly.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
util/_create_ssl_certificates.sh (1)
28-32: ⚡ Quick winAdd an explicit warning when
rootCA.pemis not found.Line 29 silently skips CA export if
"$CAROOT/rootCA.pem"is missing; that can make downstream TLS errors in dev hard to debug.💡 Suggested patch
CAROOT="$(mkcert -CAROOT)" if [ -f "$CAROOT/rootCA.pem" ]; then cp "$CAROOT/rootCA.pem" "$SSL_DIR/rootCA.pem" chmod 0644 "$SSL_DIR/rootCA.pem" +else + echo "⚠️ mkcert root CA not found at $CAROOT/rootCA.pem; Node TLS trust may fail in local dev." fiBased on learnings: Local dev Node TLS requires copying mkcert root to
util/traefik/ssl/rootCA.pemso_run_with_infisical.shcan exportNODE_EXTRA_CA_CERTS.🤖 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 `@util/_create_ssl_certificates.sh` around lines 28 - 32, The script currently silently skips exporting the mkcert CA when "$CAROOT/rootCA.pem" is missing; update the block that uses the CAROOT variable (CAROOT="$(mkcert -CAROOT)" and the subsequent check for "$CAROOT/rootCA.pem") to emit an explicit warning to stderr if the file is not found (e.g., using echo or printf to >&2) and include the paths involved ("$CAROOT/rootCA.pem" and "$SSL_DIR/rootCA.pem") and a short hint about required steps (copy rootCA.pem to util/traefik/ssl/rootCA.pem so _run_with_infisical.sh can export NODE_EXTRA_CA_CERTS), leaving the existing copy/chmod behavior unchanged when the file exists.apps/chat/src/lib/server/chatModelRegistry.ts (2)
1-5: 💤 Low valueConsider using path aliases for imports.
The import at lines 2-5 uses a relative path
'../config/reasoning'. Per coding guidelines, prefer@or~path aliases for imports within apps.♻️ Optional refactor using path alias
-import { - REASONING_EFFORT_OPTIONS, - type ReasoningEffort, -} from '../config/reasoning' +import { + REASONING_EFFORT_OPTIONS, + type ReasoningEffort, +} from '@/lib/config/reasoning'🤖 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 `@apps/chat/src/lib/server/chatModelRegistry.ts` around lines 1 - 5, The import uses a relative path for the reasoning config; update the import of REASONING_EFFORT_OPTIONS and ReasoningEffort in chatModelRegistry.ts to use the project's path alias (e.g. '@/config/reasoning' or '~ /config/reasoning') instead of '../config/reasoning', and ensure your tsconfig/webpack/Vite path mappings include that alias so the module resolves correctly.
66-115: ⚖️ Poor tradeoffCode duplication: reasoning effort logic duplicated in GraphQL service.
The constants
BASE_REASONING_EFFORTS,FRONTIER_REASONING_EFFORTS,GPT5_REASONING_EFFORTS, and the functiongetDefaultReasoningEffortsForModelare duplicated inpackages/graphql/src/services/chatbots.ts(lines 45-162).Consider extracting this shared logic into a common package (e.g.,
@klicker-uzh/shared-utilsor@klicker-uzh/types) to maintain consistency and reduce maintenance burden.🤖 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 `@apps/chat/src/lib/server/chatModelRegistry.ts` around lines 66 - 115, The constants BASE_REASONING_EFFORTS, FRONTIER_REASONING_EFFORTS, GPT5_REASONING_EFFORTS and the function getDefaultReasoningEffortsForModel are duplicated; extract them into a shared module (e.g., export from a new file in a shared package like `@klicker-uzh/shared-utils` or `@klicker-uzh/types`) and replace the local definitions in chatModelRegistry.ts and packages/graphql/src/services/chatbots.ts with imports; ensure you export the arrays and the dedupeReasoningEfforts/getDefaultReasoningEffortsForModel functions from the shared module, update all import sites to use the new module, and remove the duplicated declarations to keep a single source of truth.
🤖 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.
Nitpick comments:
In `@apps/chat/src/lib/server/chatModelRegistry.ts`:
- Around line 1-5: The import uses a relative path for the reasoning config;
update the import of REASONING_EFFORT_OPTIONS and ReasoningEffort in
chatModelRegistry.ts to use the project's path alias (e.g. '@/config/reasoning'
or '~ /config/reasoning') instead of '../config/reasoning', and ensure your
tsconfig/webpack/Vite path mappings include that alias so the module resolves
correctly.
- Around line 66-115: The constants BASE_REASONING_EFFORTS,
FRONTIER_REASONING_EFFORTS, GPT5_REASONING_EFFORTS and the function
getDefaultReasoningEffortsForModel are duplicated; extract them into a shared
module (e.g., export from a new file in a shared package like
`@klicker-uzh/shared-utils` or `@klicker-uzh/types`) and replace the local
definitions in chatModelRegistry.ts and
packages/graphql/src/services/chatbots.ts with imports; ensure you export the
arrays and the dedupeReasoningEfforts/getDefaultReasoningEffortsForModel
functions from the shared module, update all import sites to use the new module,
and remove the duplicated declarations to keep a single source of truth.
In `@util/_create_ssl_certificates.sh`:
- Around line 28-32: The script currently silently skips exporting the mkcert CA
when "$CAROOT/rootCA.pem" is missing; update the block that uses the CAROOT
variable (CAROOT="$(mkcert -CAROOT)" and the subsequent check for
"$CAROOT/rootCA.pem") to emit an explicit warning to stderr if the file is not
found (e.g., using echo or printf to >&2) and include the paths involved
("$CAROOT/rootCA.pem" and "$SSL_DIR/rootCA.pem") and a short hint about required
steps (copy rootCA.pem to util/traefik/ssl/rootCA.pem so _run_with_infisical.sh
can export NODE_EXTRA_CA_CERTS), leaving the existing copy/chmod behavior
unchanged when the file exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3647d571-b0c8-4389-9653-ae44d924d562
⛔ Files ignored due to path filters (3)
packages/graphql/src/ops.schema.jsonis excluded by!**/**/ops.schema.jsonpackages/graphql/src/ops.tsis excluded by!**/**/ops.tspackages/graphql/src/public/schema.graphqlis excluded by!**/**/public/**
📒 Files selected for processing (11)
AGENTS.mdapps/chat/src/lib/config/reasoning.tsapps/chat/src/lib/server/chatModelRegistry.tsapps/chat/src/stores/settingsStore.tsapps/chat/test/chat-model-registry.test.tsapps/frontend-manage/src/components/resources/chatbots/ChatbotDetails.tsxpackages/graphql/src/schema/resource.tspackages/graphql/src/services/chatbots.tspackages/prisma-data/src/data/seedChatbots.tsutil/_create_ssl_certificates.shutil/_run_with_infisical.sh
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
rschlaefli
added a commit
that referenced
this pull request
May 10, 2026
Combine LTI guest access (Phase A, #5083) with student practice MCP from v3-ai (#5090). Resolved apps/chat/package.json (keep both @klicker-uzh/types and @klicker-uzh/util deps), chat/route.ts (combine authMode + chatbot:authChatbot destructuring of withChatbotAuth result), and regenerated pnpm-lock.yaml.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR carries the cleaned TypeScript student MCP path against
v3-ai.apps/mcp-student, a FastMCP TypeScript server for student-facing practice workflows.@klicker-uzh/typesand imports generated GraphQL operation/persisted-query outputs.mcp-student, chatMCP_STUDENT_URLwiring, stg/prd values, and GHCR image workflows.Deployment
mcp-studentis deployed as a ClusterIP-only service. It has no external ingress.MCP_STUDENT_URLpointing to the in-cluster service.APP_SECRETfrom the existing chat secret so participant MCP JWT validation and signed question refs use the same signing material. This can be overridden viamcpStudent.secret.nameif we later split secrets.Screenshots
Before answering:
Answer selected:
After submit and grading feedback:
Verification
pnpm --filter @klicker-uzh/mcp-student testpnpm --filter @klicker-uzh/mcp-student checkpnpm --filter @klicker-uzh/mcp-student buildpnpm --filter @klicker-uzh/chat test:run -- test/student-practice-mcp.test.tspnpm --filter @klicker-uzh/chat checkpnpm --filter @klicker-uzh/shared-components checkpnpm --filter @klicker-uzh/graphql checkpnpm --filter @klicker-uzh/types checkhelm lint deploy/charts/klicker-uzh-v3 --values deploy/env-uzh-stg/values.yamlhelm lint deploy/charts/klicker-uzh-v3 --values deploy/env-uzh-prd/values.yamlhelm template klicker deploy/charts/klicker-uzh-v3 --values deploy/env-uzh-stg/values.yaml --debughelm template klicker deploy/charts/klicker-uzh-v3 --values deploy/env-uzh-prd/values.yaml --debugpnpm exec prettier --check AGENTS.md .github/workflows/v3_mcp-student-stg.yml .github/workflows/v3_mcp-student-prd.yml deploy/env-uzh-stg/values.yaml deploy/env-uzh-prd/values.yaml deploy/charts/klicker-uzh-v3/values.yamlgit diff --check origin/v3-ai..HEADpnpm run check:allsuccessfully.pnpm run buildsuccessfully.Local E2E
devsemantics, notdev:test.apps/mcp-studentagainst the seeded local database.agent-browserthat a seeded student chat can render a practice-pool quiz through the shared renderer, select an answer, submit throughPOST /api/chatbots/[chatbotId]/practice/submit, and display backend grading feedback.Notes
worktree-tsPR branch.MISSING_MESSAGEduring static generation, and local chat build warnings when OpenAI env vars are absent.Summary by CodeRabbit
New Features
Components
Deployment
Localization
Models & Reasoning