Add AI assistance tools for instructors helping students#602
Add AI assistance tools for instructors helping students#602
Conversation
This commit adds a Model Context Protocol (MCP) server that enables AI assistants to help TAs support students who are struggling with errors in their programming assignments. Features: - MCP server with Supabase OAuth authentication (instructors/graders only) - Tools for fetching help requests, discussion threads, submissions, test results, and build output - Privacy protection: never exposes users table or is_private_profile - handout_url field added to assignments table for providing assignment context to AI assistants - AI Help button added to help request chat and discussion thread views - Generates MCP context that can be used with any MCP-compatible client Components added: - packages/mcp-server/ - Standalone MCP server package - components/ai-help/AIHelpButton.tsx - UI component for launching AI help - Database migration for handout_url field https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
- Add automatic fetching of the latest student submission when retrieving help requests or discussion threads - Include all submission files in the response for complete context - Add SubmissionFileContext type with id, name, and contents - Update HelpRequestContext with student_profile_id and latest_submission - Update DiscussionThreadContext with author_profile_id and latest_submission - Add getSubmissionFiles and getLatestSubmissionForStudent functions This provides AI assistants with comprehensive context including the student's most recent code when helping with errors. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
This commit replaces the initial OAuth-based authentication with a more robust JWT-based API token system following the MCP auth specification: Database Changes: - Add api_tokens table for storing token metadata (not the tokens) - Add revoked_token_ids table for fast revocation lookups - Add trigger to populate revoked_token_ids on revocation Edge Function: - Move MCP server to /supabase/functions/mcp-server/ - Implement JWT verification for API tokens (MCP_JWT_SECRET) - Mint short-lived Supabase JWTs for RLS enforcement - Add scope-based permission checking (mcp:read, mcp:write) - Token validation requires zero DB queries for auth overhead Token Management API: - POST /api/mcp-tokens - Create new API token - GET /api/mcp-tokens - List user's tokens - DELETE /api/mcp-tokens/[id] - Revoke a token Authentication Flow: 1. User logs into dashboard (Supabase Auth) 2. User creates API token (shown once, user stores) 3. MCP client includes token in Authorization header 4. Edge function verifies JWT, mints short-lived Supabase JWT 5. All queries use RLS via the minted JWT Security: - Tokens are JWTs, not stored in database - Signing keys in environment (isolated from DB) - Revocation via revoked_token_ids lookup - Short-lived Supabase JWTs (60s TTL, cached) - Instructors/graders only access https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
Adds a new MCPTokensMenu component to the user settings drawer that allows instructors and graders to create, view, and revoke MCP API tokens. The UI includes clear instructions for configuring tokens in MCP clients. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
WalkthroughAdds an AI-assist workflow: client AI buttons/feedback and MCP token UI, two Supabase Edge Functions (mcp-server, mcp-tokens) with auth/data/tooling, DB migrations/types for tokens/feedback/handout_url, client wrappers, and integrations into test-insights, discussion, and submission result UIs. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(204,229,255,0.5)
actor Instructor as Instructor/Grader
end
participant UI as Browser UI
participant Client as Supabase Client
participant EdgeTokens as MCP-Tokens Edge Fn
participant DB as Supabase DB
Instructor->>UI: Open "API Tokens" modal
UI->>Client: mcpTokensList()
Client->>EdgeTokens: GET / (Bearer)
EdgeTokens->>EdgeTokens: authenticate + role check
EdgeTokens->>DB: SELECT api_tokens WHERE user_id=...
DB-->>EdgeTokens: token metadata
EdgeTokens-->>Client: tokens list
Client-->>UI: render tokens
Instructor->>UI: Create token (name, scopes)
UI->>Client: mcpTokensCreate()
Client->>EdgeTokens: POST / (create)
EdgeTokens->>EdgeTokens: validate, generate token
EdgeTokens->>DB: INSERT api_tokens metadata
DB-->>EdgeTokens: inserted row
EdgeTokens-->>Client: one-time token payload
Client-->>UI: show created token for copy
sequenceDiagram
rect rgba(255,229,204,0.5)
actor Instructor as Instructor/Grader
end
participant ThreadUI as Discussion/Help UI
participant AIButton as AIHelpButton (client)
participant ExternalAI as External AI / LLM
participant MCPServer as mcp-server Edge Fn
participant DB as Supabase DB
participant FeedbackRPC as submit_ai_help_feedback RPC
Instructor->>ThreadUI: Click AI help icon
ThreadUI->>AIButton: prepare prompt / copy
Instructor->>ExternalAI: paste prompt into AI chat
ExternalAI->>MCPServer: tools/list or tools/call (MCP)
MCPServer->>MCPServer: authenticateMCPRequest (token)
MCPServer->>DB: fetch contextual data (help_request/discussion/submissions)
DB-->>MCPServer: contextual data
MCPServer-->>ExternalAI: tool response / data
ExternalAI-->>Instructor: AI-assisted reply
Instructor->>AIButton: submit feedback
AIButton->>FeedbackRPC: aiHelpFeedbackSubmit(params)
FeedbackRPC->>DB: INSERT ai_help_feedback
DB-->>FeedbackRPC: OK
FeedbackRPC-->>AIButton: success
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@app/api/mcp-tokens/`[id]/route.ts:
- Around line 54-55: Replace the two console.error calls in
app/api/mcp-tokens/[id]/route.ts with your application's structured logger
(e.g., processLogger.error or logger.error) and include the error object and
contextual fields; specifically, in the token revocation block where updateError
is logged, call the structured logger with a clear message and the updateError
as metadata, and do the same for the other console.error occurrence (replace its
error variable accordingly) so linting no-console violations are removed and
logs integrate with the observability stack.
In `@app/api/mcp-tokens/route.ts`:
- Around line 102-103: Replace the console.error calls in
app/api/mcp-tokens/route.ts with your structured logger: import and use the
shared logger (e.g., appLogger.error or processLogger.error) and include
contextual messages and the error object; update each occurrence flagged (the
console.error calls around token fetch/handling and the ones at the other
flagged locations) to call logger.error("contextual message", { error: err,
extra: { /* request id or params */ } }) or remove them if logging isn't needed,
and ensure you add the corresponding import for the logger at the top of the
file.
- Around line 33-59: Add an integration test that ensures JWTs produced by
createApiToken are verifiable by the Deno/MCPAuth implementation: call
createApiToken to produce a token (using the same MCP_JWT_SECRET), then run the
Deno-side verification path in supabase/functions/_shared/MCPAuth.ts (using
getMcpJwtKey or the existing verify routine) to assert the token’s signature and
claims (sub, scopes, jti, iss, aud, exp/iat numeric format) validate; if needed,
mock or inject the same secret retrieval so the test runs deterministically
across runtimes and fails if any claim formatting or secret encoding
(TextEncoder vs getMcpJwtKey) causes incompatibility.
In `@app/course/`[course_id]/discussion/[root_id]/page.tsx:
- Around line 171-181: Remove the redundant outer Tooltip wrapper around
AIHelpIconButton in the JSX: delete the surrounding <Tooltip content="Get AI
assistance"> ... </Tooltip> and render AIHelpIconButton directly (preserving its
props: contextType="discussion_thread", resourceId={thread.id},
classId={thread.class_id}, assignmentId={topicAssignmentId ?? undefined}); this
avoids double-nesting with the internal Tooltip implemented inside the
AIHelpIconButton component.
In `@components/ai-help/AIHelpButton.tsx`:
- Around line 160-175: In AIHelpButton, React hooks (e.g., useMemo/useCallback
and any other hooks) are being invoked after the early return that checks
useIsGraderOrInstructor, which violates the Rules of Hooks; move all hook calls
(including useMemo/useCallback and any state or effect hooks) to the top of the
AIHelpButton function before calling useIsGraderOrInstructor, then keep the
existing conditional that returns null for non-instructors/graders purely as a
render-time return; ensure you preserve hook dependencies and names (e.g., any
callbacks/memoized values used later) when relocating them above the
isInstructorOrGrader check.
- Around line 276-288: The AIHelpIconButton function violates the Rules of Hooks
by calling hooks conditionally; ensure every React hook (e.g.,
useIsGraderOrInstructor and any other useX hooks inside AIHelpIconButton) is
invoked unconditionally at the top of the component before the early return,
then keep the existing conditional "if (!isInstructorOrGrader) return null;"
only after all hooks have been called so no hook calls occur after a conditional
return.
In `@components/settings/MCPTokensMenu.tsx`:
- Around line 181-184: The trigger button is currently rendered while
isAuthorized is null, causing non-staff users to see and open the dialog before
authorization is known; update the MCPTokensMenu component to gate rendering
using the existing useIsGraderOrInstructor hook (the same pattern as
AIHelpIconButton) or ensure authorization is fetched on mount and only render
the trigger button when isAuthorized is true (or the hook returns positive).
Specifically, replace the current early-return check around isAuthorized with
the hook-based check (or call the async authorization fetch in useEffect and
block rendering until resolved) so the trigger button and dialog cannot be shown
to users before authorization is determined.
In `@packages/mcp-server/src/data.ts`:
- Around line 603-607: The filter currently injects raw options.searchQuery into
query.or (subject.ilike and body.ilike) which allows PostgREST filter-string
injection and unescaped SQL wildcards; fix by sanitizing and escaping the search
term before building the filter: (1) escape PostgREST-reserved characters (',',
'.', ':', '(', ')') in options.searchQuery, (2) escape SQL ILIKE wildcards '%'
and '_' by prefixing with an escape character and then wrap the escaped term
with literal % for a contains match, and (3) construct the query.or call using
the sanitized term for both subject.ilike and body.ilike (referencing query.or,
options.searchQuery, subject.ilike, body.ilike) so no raw unescaped user input
is inserted into the filter expression; alternatively switch to a server-side
RPC that performs parameterized LIKE matching if available.
In `@packages/mcp-server/src/index.ts`:
- Around line 83-96: The redirect uses the unvalidated state -> redirectUri in
the express handler (app.get("/auth/callback", async (req, res) => { ... }))
causing an open-redirect; validate and sanitize the state before redirecting:
treat state as a permitted redirect only if it is a relative path (starts with
"/") or it exactly matches an entry in a configured allowlist of client redirect
origins (environment or config), otherwise fall back to a safe default ("/").
Use URL parsing to detect absolute URLs/hosts and reject external hosts; ensure
you decode state only after validation and always construct the redirect with a
safe path, e.g. append ?code=... to the validated relative path or allowlisted
URL.
- Around line 31-80: The /mcp route (app.post("/mcp")) lacks rate limiting;
create and apply an express-rate-limit middleware instance (import from
"express-rate-limit") with a sensible window and max (e.g., short window and
limited requests per IP) and attach it to the route so requests hit the limiter
before authenticateRequest/handler logic; ensure the limiter is created once
(e.g., mcpRateLimiter) and used as middleware in the app.post("/mcp",
mcpRateLimiter, async (req, res) => { ... }) call so
transport/StreamableHTTPServerTransport and transport.handleRequest only run
after rate checks pass.
In `@supabase/functions/mcp-server/index.ts`:
- Around line 578-580: The current use of options.searchQuery directly in
query.or('subject.ilike.%...%,body.ilike.%...%') can be abused via wildcard
characters (% and _) or backslashes; sanitize options.searchQuery first by
escaping backslash, percent and underscore characters (e.g., replace \ with \\
then % with \% and _ with \_) and then wrap the escaped string with surrounding
% for substring matching, and use that safe pattern when calling query.or (refer
to options.searchQuery and the query.or / subject.ilike / body.ilike usage) so
raw user input never becomes an unescaped pattern.
🧹 Nitpick comments (17)
packages/mcp-server/package.json (2)
1-27: Consider adding ESMexportsfield and marking package as private.A few optional improvements for better package hygiene:
- Add an
"exports"field for explicit ESM entry point resolution.- If this package isn't meant to be published to npm, add
"private": trueto prevent accidental publishing.📦 Suggested package.json enhancements
{ "name": "@pawtograder/mcp-server", "version": "0.1.0", "description": "Pawtograder MCP Server for AI-assisted student help", + "private": true, "type": "module", "main": "dist/index.js", + "exports": { + ".": { + "import": "./dist/index.js", + "types": "./dist/index.d.ts" + } + }, "scripts": {Note: If you do add the
"types"export, ensure yourtsconfig.jsonhas"declaration": trueto generate.d.tsfiles.
12-17: Consider tightening version constraints for@supabase/supabase-js.The
@supabase/supabase-jsversion^2.45.0is quite outdated; current stable releases are at2.93.x. Update to^2.93.0to use more recent bug fixes and improvements. Thezod ^3.25.0and@modelcontextprotocol/sdk ^1.0.0versions are valid and available on npm.packages/mcp-server/tsconfig.json (1)
2-15: Consider adding useful optional compiler options.While the current configuration is solid, consider adding these options for improved developer experience:
"resolveJsonModule": true— enables importing JSON files (useful for config files)"isolatedModules": true— ensures each file can be transpiled independently (recommended for build tools)"lib": ["ES2022"]— explicitly declares which built-in APIs are available➕ Suggested additions
"declaration": true, "declarationMap": true, - "sourceMap": true + "sourceMap": true, + "resolveJsonModule": true, + "isolatedModules": true, + "lib": ["ES2022"] },packages/mcp-server/README.md (1)
16-60: Consider adding a language identifier to the fenced code block.The ASCII diagram could have a language identifier like
textorplaintextto satisfy linting rules, though this is purely cosmetic.📝 Optional fix
-``` +```text ┌─────────────────────────────────────────────────────────────────────────┐packages/mcp-server/src/types.ts (1)
140-145: Consider typing the supabase client more specifically.Using
unknownfor the supabase client works but loses type safety. You could import and use the actualSupabaseClienttype.📝 Optional improvement
+import type { SupabaseClient } from '@supabase/supabase-js'; + // MCP context that gets passed to tools export interface MCPContext { - supabase: unknown; // SupabaseClient type + supabase: SupabaseClient; userId: string; userRoles: UserRole[]; }supabase/migrations/20260130031926_create_api_tokens_table.sql (1)
25-26: Redundant index on token_id.The
UNIQUEconstraint ontoken_id(Line 10) already creates an implicit index. The explicitidx_api_tokens_token_idindex is redundant and adds unnecessary overhead during writes.🔧 Suggested removal
-- Create index for fast user lookups CREATE INDEX IF NOT EXISTS idx_api_tokens_user_id ON public.api_tokens(user_id); - --- Create index for token_id lookups (revocation checks) -CREATE INDEX IF NOT EXISTS idx_api_tokens_token_id ON public.api_tokens(token_id);supabase/functions/_shared/MCPAuth.ts (2)
71-84: Consider adding minimum length validation for SUPABASE_JWT_SECRET.
getMcpJwtKey()validates thatMCP_JWT_SECRETis at least 32 characters (Line 55-56), butgetSupabaseJwtKey()lacks this validation. For consistency and security, apply the same minimum length check here.🔧 Suggested fix
async function getSupabaseJwtKey(): Promise<CryptoKey> { const secret = Deno.env.get(SUPABASE_JWT_SECRET_ENV); - if (!secret) { - throw new Error(`${SUPABASE_JWT_SECRET_ENV} must be set`); + if (!secret || secret.length < 32) { + throw new Error(`${SUPABASE_JWT_SECRET_ENV} must be set and at least 32 characters`); }
157-161: Non-null assertions on environment variables risk unclear runtime errors.Using
!forSUPABASE_URLandSUPABASE_SERVICE_ROLE_KEYassumes they're always set. If missing, you'll get a cryptic error fromcreateClientrather than a clear "env var not configured" message.Consider adding explicit validation:
🔧 Suggested defensive pattern
export async function isTokenRevoked(tokenId: string): Promise<boolean> { + const supabaseUrl = Deno.env.get(SUPABASE_URL_ENV); + const serviceRoleKey = Deno.env.get("SUPABASE_SERVICE_ROLE_KEY"); + if (!supabaseUrl || !serviceRoleKey) { + throw new Error("SUPABASE_URL and SUPABASE_SERVICE_ROLE_KEY must be set"); + } + const adminSupabase = createClient<Database>( - Deno.env.get(SUPABASE_URL_ENV)!, - Deno.env.get("SUPABASE_SERVICE_ROLE_KEY")! + supabaseUrl, + serviceRoleKey );This pattern should also be applied to similar usages at lines 268-271 and 327-330.
app/api/mcp-tokens/route.ts (2)
16-22: Code duplication with MCPAuth.ts.
MCP_TOKEN_PREFIX,MCPScope, andVALID_SCOPESare defined here and also insupabase/functions/_shared/MCPAuth.ts(lines 22-26). If these definitions drift apart, token creation/verification could break.Consider extracting shared constants to a common location or importing from a shared types package.
79-92: Duplicated role check logic.The instructor/grader role verification (lines 79-92) is duplicated in POST (lines 131-144). Extract to a helper function.
🔧 Suggested refactor
async function verifyInstructorOrGrader(supabase: SupabaseClient): Promise<boolean> { const { data: roles } = await supabase .from("user_roles") .select("class_id, role") .eq("user_id", (await supabase.auth.getUser()).data.user?.id) .eq("disabled", false) .in("role", ["instructor", "grader"]); return roles && roles.length > 0; }supabase/functions/mcp-server/index.ts (1)
170-173: Significant code duplication with packages/mcp-server/src/data.ts.The data access functions (lines 175-621) appear to duplicate the implementations in
packages/mcp-server/src/data.ts. This creates maintenance burden and risks the implementations drifting apart.Consider importing from the shared package or using a build step to share code between the edge function and the package.
components/settings/MCPTokensMenu.tsx (1)
169-175: Consider using shared date formatting utility.A
formatDatefunction exists inutils/time-formatting.ts. While the implementations differ slightly (yours includes more formatting options), consider unifying date formatting across the codebase for consistency.packages/mcp-server/src/server.ts (2)
85-87: Consider adding runtime validation for context to improve robustness.The type assertion
extra.context as { supabase: SupabaseClient; roles: UserRole[] }assumes the context shape is correct. If context is missing or malformed (e.g., due to transport changes), this will fail silently or throw cryptic errors.♻️ Suggested defensive pattern
+import { z } from "zod"; + +const ToolContextSchema = z.object({ + supabase: z.any(), // Can't validate SupabaseClient shape with zod easily + roles: z.array(z.object({ + class_id: z.number(), + role: z.enum(["instructor", "grader", "student"]), + private_profile_id: z.string(), + })), +}); + +function getToolContext(extra: { context?: unknown }) { + const result = ToolContextSchema.safeParse(extra.context); + if (!result.success) { + throw new Error("Invalid tool context"); + } + return result.data as { supabase: SupabaseClient; roles: UserRole[] }; +} + // Then in each tool handler: -const { supabase, roles } = extra.context as { supabase: SupabaseClient; roles: UserRole[] }; +const { supabase, roles } = getToolContext(extra);
146-151: MissingincludeFilesparameter forgetSubmissioncall.The
getSubmissionfunction signature indata.tsaccepts bothincludeTestOutputandincludeFilesparameters. Here onlyinclude_test_outputfrom the schema is passed, butincludeFilesdefaults totrue. This is likely the desired behavior, but it's inconsistent with the schema which only exposesinclude_test_output.Consider either:
- Adding
include_filesto the schema for consistency- Explicitly passing
trueto document the intent♻️ Option 1: Add include_files to schema
const GetSubmissionSchema = z.object({ submission_id: z.number().describe("The ID of the submission to fetch"), class_id: z.number().describe("The class ID where the submission exists"), include_test_output: z.boolean().optional().default(true).describe("Whether to include test output"), + include_files: z.boolean().optional().default(true).describe("Whether to include submission files"), });And update the call:
const submission = await getSubmission( supabase, input.submission_id, input.class_id, - input.include_test_output + input.include_test_output, + input.include_files );packages/mcp-server/src/data.ts (2)
225-246: Performance: N+1 query pattern in message/reply processing loops.Both
getHelpRequest(lines 225-246) andgetDiscussionThread(lines 322-344) execute 2 queries per item in a loop:
getProfileName→ profile lookup- Role check query → user_roles lookup
For a help request with 20 messages, this results in 40+ additional queries.
♻️ Suggested batch optimization
+// Batch fetch all profile names and roles upfront +const profileIds = messagesData.map(m => m.profile_id).filter(Boolean); +const uniqueProfileIds = [...new Set(profileIds)]; + +// Fetch all profiles in one query +const { data: profiles } = await supabase + .from("profiles") + .select("id, name") + .in("id", uniqueProfileIds); + +// Fetch all roles in one query +const { data: allRoles } = await supabase + .from("user_roles") + .select("private_profile_id, role") + .in("private_profile_id", uniqueProfileIds) + .eq("class_id", classId) + .in("role", ["instructor", "grader"]); + +const profileNameMap = new Map(profiles?.map(p => [p.id, p.name]) || []); +const staffSet = new Set(allRoles?.map(r => r.private_profile_id) || []); + for (const msg of messagesData) { - const authorName = await getProfileName(supabase, msg.profile_id); - const { data: roleData } = await supabase... + const authorName = profileNameMap.get(msg.profile_id) || null; + const isStaff = staffSet.has(msg.profile_id); messages.push({ ... - author_name: authorName, - is_staff: !!roleData, + author_name: authorName, + is_staff: isStaff, }); }Also applies to: 322-344
498-506: Performance: Sequential fetching in loops causes N+1 queries.
getSubmissionsForStudentandsearchHelpRequestsfetch full objects one-by-one in a loop. For a student with 10 submissions, this causes 10 separategetSubmissioncalls, each potentially making 4-5 queries internally.For the search functions, consider whether returning summary data (IDs, titles, timestamps) is sufficient for the initial results, with full details fetched on-demand.
Also applies to: 539-552
packages/mcp-server/src/index.ts (1)
57-59: Remove non-idiomatic_contextinjection from request params.The MCP SDK provides a standard pattern for passing request-scoped context: tool handlers receive an
extraparameter containingrequestId,signal, and optionallysessionId. For application-specific context (auth/user/session data), the idiomatic approach is to attach it at the transport layer and look it up in handlers usingextra.sessionIdor closure-captured state.Injecting
_contextintomcpRequest.paramsdiverges from SDK conventions. Remove lines 57–59 and instead pass context through the handler'sextraparameter or attach it to the transport/middleware layer for handlers to access via the established pattern.
The MCP server is now fully implemented as a Supabase Edge Function at /supabase/functions/mcp-server/. The standalone Node.js package is no longer needed. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
- Replace console.log with Sentry logging in MCP token routes - Use built-in Node.js crypto for JWT signing instead of jose - Fix React hooks called conditionally in AIHelpButton - Replace non-existent ClipboardIconButton with standard IconButton - Add api_tokens table types to SupabaseTypes.d.ts - Format all files with prettier https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
Replace /=+$/ with /=/g to avoid potential ReDoS. Since = only appears as padding at the end of base64 strings, this is functionally equivalent. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/api/mcp-tokens/route.ts`:
- Around line 188-191: The current use of || to set expiresInDays masks invalid
values like 0 or NaN; change the assignment to use nullish coalescing: const
expiresInDays = body.expires_in_days ?? DEFAULT_EXPIRY_DAYS; then validate with
Number.isFinite (or !Number.isFinite(Number(expiresInDays)) if you need to
coerce strings) and the existing range check: if
(!Number.isFinite(expiresInDays) || expiresInDays < 1 || expiresInDays > 365)
return the 400 response; update references to expiresInDays and keep
DEFAULT_EXPIRY_DAYS unchanged.
In `@supabase/functions/mcp-server/index.ts`:
- Around line 631-639: The class access lookup using
context.supabase.from("user_roles").select(...).eq("class_id",
classId).in("role", ["instructor", "grader"]).maybeSingle() is not bound to the
caller; add an explicit user constraint by filtering on the current user's id
(e.g., the auth user id available on the request context) so the query checks
both class_id and user_id (or equivalent field) before trusting RLS—update the
query to include .eq("user_id", <current user id from context>) to make the
check self-contained (use the same classId / args.class_id and context auth
object in your code).
🧹 Nitpick comments (2)
components/ai-help/AIHelpButton.tsx (1)
34-34: Consider a more specific return type for better type safety.The
objectreturn type is quite generic. Defining an interface for the MCP context structure would provide better IntelliSense support and catch potential typos in property names at compile time.♻️ Optional: Add explicit return type
interface MCPContext { mcp_server: string; version: string; context_type: "help_request" | "discussion_thread"; resource_id: number; class_id: number; tool: string; params: Record<string, unknown>; suggested_tools: Array<{ tool: string; params: Record<string, unknown>; description?: string; }>; } function generateMCPContext(props: AIHelpButtonProps): MCPContext { // ... }supabase/functions/_shared/MCPAuth.ts (1)
124-146: Validate theissclaim during token verification.You already check
audand scopes; add an issuer check to fully lock the JWT claim set.Suggested fix
// Validate audience if (payload.aud !== "mcp") { console.error("Invalid audience in API token"); return null; } + // Validate issuer + if (payload.iss !== "pawtograder") { + console.error("Invalid issuer in API token"); + return null; + }
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
- Remove unused hasScope import - Replace console.error with Sentry.captureException in error handlers - Add component tags for better error tracking https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@supabase/functions/mcp-server/index.ts`:
- Around line 449-480: The replies loop causes N+1 queries: each reply calls
getProfileName and queries user_roles inside the for loop (see repliesData
processing and replies.push). Fix by batching: collect all reply.author IDs from
repliesData, fetch all profiles in one call (replacing repeated getProfileName
calls) and fetch all relevant user_roles with a .in("private_profile_id",
authorIds) and .eq("class_id", classId) to determine staff roles, build maps
from id -> name and id -> is_staff, then iterate repliesData to populate the
replies array using those maps and set is_answer by comparing thread.answer to
reply.id. Ensure you keep the same fields (id, body, created_at, author_name,
is_staff, is_answer).
- Around line 518-531: The loop in searchHelpRequests calls getHelpRequest for
each id causing N+1 queries; change to a two-phase approach: implement a bulk
summary query (e.g., new function fetchHelpRequestSummaries) that selects the
help_request rows by the ids returned, joins/aggregates needed lightweight
fields (submission id/status, assignment id, message counts, timestamps) in one
or few queries, apply options.assignmentId and options.limit to that summary
set, then only call getHelpRequest for the small subset of selected IDs to
hydrate full details; update searchHelpRequests to use fetchHelpRequestSummaries
(or add an optional parameter to getHelpRequest to accept preloaded summary
data) and avoid calling getHelpRequest inside the for-loop for every id.
- Line 148: The limit parameter currently allows arbitrarily large values which
can cause huge queries; update every schema/validator that defines the limit
field (the objects where you currently have "limit: { type: \"number\",
description: \"Maximum number of results\", default: 20 }") to include a maximum
(e.g., "maximum: 100") and additionally add a runtime clamp/validation where
that limit is consumed (the request handler or query function that reads the
limit before executing DB calls) to enforce Math.min(limit, 100) or return a
validation error if limit > 100; apply this change to all occurrences of the
limit definition and the corresponding handlers so queries can never request
more than the sensible cap.
- Around line 375-395: The loop over messagesData causes an N+1 query pattern by
calling getProfileName(supabase, msg.profile_id) and querying
supabase.from("user_roles") for each msg; instead, collect all unique profile_id
values from messagesData, fetch profiles in one bulk query and fetch user_roles
for those profile_ids (and classId) with a single .in(...) query, build maps
keyed by profile_id for profile names and role existence, then iterate
messagesData to push objects into messages using those maps to set author_name
and is_staff (update references to getProfileName and the per-message user_roles
lookup accordingly).
🧹 Nitpick comments (2)
supabase/functions/mcp-server/index.ts (2)
804-805: Silently swallowing errors hides potential issues.The empty
.catch(() => {})discards any errors fromupdateTokenLastUsed. While fire-and-forget is appropriate here, consider at least logging or reporting failures to Sentry for observability.♻️ Suggested fix
// Update last used timestamp asynchronously - updateTokenLastUsed(context.tokenId).catch(() => {}); + updateTokenLastUsed(context.tokenId).catch((err) => { + Sentry.captureException(err, { + tags: { component: "mcp_server", operation: "update_token_last_used" }, + level: "warning" + }); + });
807-811: Consider validating MCP request structure before processing.The parsed JSON is cast directly to
MCPRequestwithout validation. Malformed requests (missingjsonrpc,method, etc.) could cause confusing errors downstream. A lightweight schema check would provide clearer error messages.🛡️ Example validation
function validateMCPRequest(data: unknown): MCPRequest { if ( typeof data !== "object" || data === null || (data as MCPRequest).jsonrpc !== "2.0" || typeof (data as MCPRequest).method !== "string" ) { throw new Error("Invalid MCP request: missing required fields"); } return data as MCPRequest; }
Major performance improvements: - Batch fetch profile names instead of N+1 queries per message/reply - Batch fetch staff roles instead of N+1 queries per message/reply - Use JOINs to fetch profile names with help requests and threads - Parallelize independent queries with Promise.all - Add MAX_ROWS constant (1000) for pagination limits - Search functions return lightweight results without deep fetching - getSubmissionsForStudent fetches all submissions in parallel Reduces database round-trips from O(n*m) to O(1) for most operations. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
- Create ai_help_feedback table with RLS policies - Add API endpoint for submitting thumbs up/down + comments - Show feedback panel after copying AI context in AIHelpButton - Update SupabaseTypes with ai_help_feedback table types https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@supabase/functions/_shared/SupabaseTypes.d.ts`:
- Around line 99-134: The generated api_tokens type is missing its foreign-key
relationship—update the api_tokens entry so its Relationships array includes the
link to auth.users via user_id (reflecting the migration that added REFERENCES
auth.users(id) ON DELETE CASCADE); locate the api_tokens type block
(Row/Insert/Update/Relationships) and add a Relationship entry referencing
user_id -> auth.users(id) with the appropriate fields/name so that typed
relational queries recognize the user relation.
- Around line 135-175: The ai_help_feedback type is missing the foreign-key
relationship for user_id in its Relationships array; update the Relationships
for ai_help_feedback to include a relationship object with foreignKeyName
"ai_help_feedback_user_id_fkey", columns ["user_id"], isOneToOne: false,
referencedRelation "auth.users" and referencedColumns ["id"] so type-safe joins
against auth.users(id) work (modify the Relationships array in the
ai_help_feedback entry in SupabaseTypes.d.ts or regenerate types from the DB
schema).
In `@supabase/migrations/20260130170038_create_ai_help_feedback_table.sql`:
- Around line 39-52: The SELECT policy "Instructors can view class feedback" on
public.ai_help_feedback currently only allows users where user_roles.role =
'instructor'; update the USING clause to also permit role = 'grader' (e.g.,
check ur.role IN ('instructor','grader')) so graders can view feedback, and if
feedback should be mutable add corresponding CREATE POLICY blocks for UPDATE and
DELETE on public.ai_help_feedback (or explicitly document immutability) to
define who may modify or remove rows; refer to the policy name "Instructors can
view class feedback", the ai_help_feedback table, and the user_roles alias ur
when making the change.
In `@utils/supabase/SupabaseTypes.d.ts`:
- Around line 135-175: The Supabase types for ai_help_feedback are missing the
user_id foreign-key relationship; update the Relationships array for
ai_help_feedback to include an entry for the user_id FK (e.g. foreignKeyName
like "ai_help_feedback_user_id_fkey") that maps columns: ["user_id"] to
referencedRelation: "auth.users" (or "users" depending on your generator) and
referencedColumns: ["id"]; regenerate the Supabase types or add this
relationship entry alongside the existing class_id relationship so TypeScript
correctly understands joins against user data for ai_help_feedback.
🧹 Nitpick comments (2)
supabase/migrations/20260130170038_create_ai_help_feedback_table.sql (1)
2-11: Consider adding a unique constraint to prevent duplicate feedback submissions.Without a unique constraint on
(user_id, context_type, resource_id), users could submit multiple feedback entries for the same resource. If you want to allow only one rating per user per resource (with ability to change it later), add a unique constraint. If multiple submissions are intentional (e.g., feedback over time), this can be ignored.🛠️ Proposed fix to add unique constraint
comment TEXT, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() ); + +-- Prevent duplicate feedback from the same user on the same resource +CREATE UNIQUE INDEX idx_ai_help_feedback_unique_user_resource + ON public.ai_help_feedback(user_id, context_type, resource_id);app/api/ai-help-feedback/route.ts (1)
67-78: Consider handling query errors explicitly.The role check doesn't distinguish between "no roles found" and "query failed with an error." If the database query fails, the user gets a 403 ("Feedback is only available to instructors and graders") instead of a 500, which masks the actual issue.
🛡️ Proposed fix to handle query errors
// Check if user has instructor/grader role in this class - const { data: roles } = await supabase + const { data: roles, error: rolesError } = await supabase .from("user_roles") .select("role") .eq("user_id", user.id) .eq("class_id", body.class_id) .eq("disabled", false) .in("role", ["instructor", "grader"]); + if (rolesError) { + Sentry.captureException(rolesError, { + tags: { endpoint: "ai_help_feedback", operation: "role_check" } + }); + return NextResponse.json({ error: "Internal server error" }, { status: 500 }); + } + if (!roles || roles.length === 0) { return NextResponse.json({ error: "Feedback is only available to instructors and graders" }, { status: 403 }); }
supabase/migrations/20260130170038_create_ai_help_feedback_table.sql
Outdated
Show resolved
Hide resolved
- Create AIHelpTestErrorButton component for analyzing common error patterns - Integrate button into CommonErrorsExplorer error cards - Generate MCP-compatible prompts with error details and suggested tools - Pass assignmentId and classId from test insights page https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
…posts - Focus on two use cases: identifying assignment/test bugs and drafting discussion posts - Add assessment section for detecting spec contradictions or unclear tests - Provide ready-to-copy markdown format for pinnable discussion posts - Remove unused MCP context variable https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/test-insights/AIHelpTestErrorButton.tsx`:
- Around line 139-143: The icon-only button in the AIHelpTestErrorButton
component lacks an accessible name when size === "xs"; update the Button (inside
Tooltip) to provide an aria-label (or visually-hidden text) when rendering
icon-only so screen readers can discover it—for example, add aria-label={size
=== "xs" ? "AI Analyze" : undefined} (or compute a localized label) on the
Button that contains the Icon and ensure this does not duplicate the visible
label when size !== "xs"; references: AIHelpTestErrorButton, handleCopy,
Tooltip, Button, Icon.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
- Create mcp-tokens Edge Function for token management (GET/POST/DELETE) - Create ai-help-feedback Edge Function for feedback submission - Add edge function wrappers to lib/edgeFunctions.ts - Update AIHelpButton to use Edge Function via Supabase client - Remove Next.js API routes (/api/mcp-tokens, /api/ai-help-feedback) All server functions are now Deno-based Supabase Edge Functions. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/edgeFunctions.ts`:
- Around line 447-451: The interface AIHelpFeedbackResponse declares feedback_id
as a string but the edge function returns feedback.id (a numeric DB id); update
the type to match the actual returned value—change
AIHelpFeedbackResponse.feedback_id to number (or alternatively ensure the edge
function serializes feedback.id to a string) so the types align with the value
returned by the edge function.
In `@supabase/functions/ai-help-feedback/index.ts`:
- Around line 183-190: The response error handling currently maps only the exact
message "Unauthorized" to 401, causing "Missing Authorization header" to return
500; in the catch block where message is derived from error (variable message)
and status is set, update the status logic (used in the return Response) to
treat "Missing Authorization header" as 401 as well—either by checking message
=== "Unauthorized" || message === "Missing Authorization header" or by using
message.includes("Authorization") so both the thrown "Missing Authorization
header" (thrown earlier) and "Unauthorized" result in status 401 before
returning the JSON error response with corsHeaders and Content-Type.
In `@supabase/functions/mcp-tokens/index.ts`:
- Around line 245-252: The insert into the revoked_token_ids table using
adminSupabase.insert({ token_id: body.token_id }).single() lacks error handling;
check the result/error from that insert call and handle failures (e.g., if
insert returns an error) by either retrying, logging and returning a 500, or
rolling back the prior update to api_tokens so you don't leave a partially
revoked state—update the code around the
adminSupabase.from("revoked_token_ids").insert(...) call to inspect the returned
data/error, and on error perform a compensating action (rollback the api_tokens
revoke or abort and surface the error) and log the error with context including
body.token_id and the error message.
- Create submit_ai_help_feedback RPC function with SECURITY DEFINER - Validates user is instructor/grader before inserting - Update edgeFunctions.ts to call RPC instead of Edge Function - Remove ai-help-feedback Edge Function RPC is simpler and more efficient for this CRUD operation. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/edgeFunctions.ts`:
- Around line 367-375: MCPToken.id is declared as number but the DB schema
(api_tokens.Row.id) is a string (UUID); update the MCPToken interface to use id:
string to match the SupabaseTypes definition and ensure any code assuming a
numeric id (parsing, comparisons, or arithmetic) in functions that consume
MCPToken (e.g., where MCPToken is constructed or read) is adjusted to treat id
as a string.
🧹 Nitpick comments (2)
supabase/migrations/20260131000000_ai_help_feedback_rpc.sql (2)
42-51: Authorization check is solid, but consider validating resource ownership.The instructor/grader role check is well-implemented with the
disabled = falsecondition. However, the function doesn't verify thatp_resource_idactually belongs top_class_id. A caller could potentially submit feedback associating a resource from one class with a different class_id, leading to data inconsistency.Consider adding a validation step that confirms the help_request or discussion_thread with
p_resource_idbelongs top_class_idbefore inserting.♻️ Proposed validation addition
IF NOT EXISTS ( SELECT 1 FROM public.user_roles WHERE user_id = v_user_id AND class_id = p_class_id AND disabled = false AND role IN ('instructor', 'grader') ) THEN RETURN json_build_object('error', 'Feedback is only available to instructors and graders'); END IF; + -- Validate resource belongs to the class + IF p_context_type = 'help_request' THEN + IF NOT EXISTS ( + SELECT 1 FROM public.help_requests + WHERE id = p_resource_id AND class_id = p_class_id + ) THEN + RETURN json_build_object('error', 'Help request not found in this class'); + END IF; + ELSIF p_context_type = 'discussion_thread' THEN + IF NOT EXISTS ( + SELECT 1 FROM public.discussion_threads + WHERE id = p_resource_id AND class_id = p_class_id + ) THEN + RETURN json_build_object('error', 'Discussion thread not found in this class'); + END IF; + END IF; + -- Insert feedback
53-69: INSERT lacks explicit error handling for database failures.If the INSERT fails (e.g., due to a constraint violation or connection issue), the error will propagate as an unhandled exception rather than returning a structured JSON error response like the other validation checks.
🛡️ Proposed exception handling
-- Insert feedback + BEGIN INSERT INTO public.ai_help_feedback ( user_id, class_id, context_type, resource_id, rating, comment ) VALUES ( v_user_id, p_class_id, p_context_type, p_resource_id, p_rating, NULLIF(trim(p_comment), '') ) RETURNING id INTO v_feedback_id; + EXCEPTION WHEN OTHERS THEN + RETURN json_build_object('error', 'Failed to save feedback'); + END;
Security: - Add sanitizeForPostgrestFilter() to escape SQL wildcards and PostgREST reserved chars in search queries (prevents filter-string injection) - Add minimum length validation for SUPABASE_JWT_SECRET (32 chars) Code Quality: - Update MCPTokensMenu to use useIsGraderOrInstructor hook for auth - Update MCPTokensMenu to use Edge Function API via supabase client - Include token_id in GET response for revocation support - Remove redundant index on token_id (UNIQUE constraint creates one) UX: - Remove redundant outer Tooltip wrapper on AIHelpIconButton (component already has internal tooltip) Note: React hooks violations were already fixed in previous commits. Note: packages/mcp-server was removed, so related feedback is N/A. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@utils/supabase/SupabaseTypes.d.ts`:
- Around line 11656-11663: The call in ErrorPinModal.tsx is passing null for
p_assignment_id when isClassLevel is true, but preview_error_pin_matches.Args
declares p_assignment_id as non-nullable; fix by either (A) updating the call in
ErrorPinModal.tsx to always pass a valid assignmentId (e.g., p_assignment_id:
assignmentId or derive a valid id when isClassLevel is true) and remove the null
branch, or (B) if the DB function truly accepts null, update the generated type
preview_error_pin_matches.Args to allow p_assignment_id: number | null so the
signature matches the database; choose one consistent approach and update the
referenced symbol (p_assignment_id in the preview_error_pin_matches Args or the
call site in ErrorPinModal.tsx where isClassLevel and assignmentId are used).
- Add index signature to MCPApiTokenPayload for djwt Payload compatibility - Remove unused 'decode' import Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Cast p_assignment_id to number to satisfy RPC type requirements when the value may be undefined for class-level error pins. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/functions/_shared/MCPAuth.ts`:
- Around line 91-109: createApiToken currently accepts arbitrary scopes which
can produce invalid tokens; update createApiToken to validate and de-duplicate
scopes before signing: filter the incoming scopes against the canonical
whitelist/enum of allowed values (e.g., MCPScope or a new ALLOWED_MCP_SCOPES
set), remove duplicates, ensure the resulting array is non-empty (throw a clear
error if empty/contains invalid entries), and use the cleaned scopes array when
building the MCPApiTokenPayload (jti, iss, aud, exp, iat remain unchanged).
🧹 Nitpick comments (1)
supabase/functions/_shared/MCPAuth.ts (1)
189-217: Bound the Supabase JWT cache to avoid unbounded growth.
supabaseJwtCachenever prunes entries, so a high number of unique users can leak memory over time. Consider TTL cleanup and a max size cap.♻️ Suggested fix
-// Cache for minted Supabase JWTs (per user_id) -const supabaseJwtCache = new Map<string, { jwt: string; expiresAt: number }>(); +// Cache for minted Supabase JWTs (per user_id) +const supabaseJwtCache = new Map<string, { jwt: string; expiresAt: number }>(); +const SUPABASE_JWT_CACHE_MAX = 1000; + +function pruneSupabaseJwtCache(now: number) { + for (const [userId, entry] of supabaseJwtCache) { + if (entry.expiresAt <= now) supabaseJwtCache.delete(userId); + } + if (supabaseJwtCache.size > SUPABASE_JWT_CACHE_MAX) { + let toRemove = supabaseJwtCache.size - SUPABASE_JWT_CACHE_MAX; + for (const key of supabaseJwtCache.keys()) { + supabaseJwtCache.delete(key); + if (--toRemove <= 0) break; + } + } +} @@ export async function mintSupabaseJwt(userId: string): Promise<string> { const now = Date.now(); + pruneSupabaseJwtCache(now);
- Create AIHelpSubmissionErrorButton component for analyzing individual test failures or build errors - Integrate with submission results page: - Add button to failing test CardHeader - Add button to GenericBuildError component - Generate MCP-compatible prompts with submission context - Only visible to instructors/graders Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create shared AIHelpFeedbackPanel component for collecting feedback - Update AIHelpButton and AIHelpIconButton to use shared panel - Update AIHelpSubmissionErrorButton to show feedback after copying - Update AIHelpTestErrorButton to show feedback after copying - Add new migration to support additional context types: test_failure, build_error, test_insights - Update RPC to validate submissions for test_failure/build_error Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@app/course/`[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx:
- Line 44: The file fails CI due to Prettier formatting; run Prettier on the
file that imports AIHelpSubmissionErrorButton (e.g., page.tsx) and apply the
formatter to fix spacing/line breaks and any trailing commas or semicolons so
the import line and surrounding code conform to the project Prettier rules;
after formatting, re-run lint/CI to confirm the Prettier error is resolved.
- Around line 1123-1146: The isFailing check treats null scores as 0 and can
mark unscored tests as failing; update the logic in the component (where
isFailing is computed) to require both result.score and result.max_score be
non-null and that result.max_score > 0 and result.score < result.max_score
(e.g., use result.score != null && result.max_score != null && result.max_score
> 0 && result.score < result.max_score) so AIHelpSubmissionErrorButton is only
shown for actually scored failures; keep passing score/maxScore as undefined
when null as done in AIHelpSubmissionErrorButton props.
In `@components/ai-help/AIHelpSubmissionErrorButton.tsx`:
- Line 1: The file AIHelpSubmissionErrorButton.tsx has a Prettier formatting
error; run your project's formatter (e.g., npm run format or prettier --write
components/ai-help/AIHelpSubmissionErrorButton.tsx) or apply your editor's
autoformat to reformat the file and commit the updated formatting so CI passes;
ensure the AIHelpSubmissionErrorButton component file is saved with Prettier
rules applied before pushing.
- Around line 85-92: The prompt currently interpolates testName, testPart,
score, and maxScore directly and can render "undefined" or wrong status; update
the generator in AIHelpSubmissionErrorButton.tsx to apply safe fallbacks (e.g.,
testName ?? "Unknown Test", testPart ? `(Part: ${testPart})` : "", and display
score and maxScore as either numeric values or "N/A" using nullish coalescing),
and compute status with an explicit check that treats only a defined numeric
score === 0 as "Failing completely", otherwise "Partial credit" or "Unknown"
when score is undefined; use the variables testName, testPart, score, and
maxScore and perform coerce/checks before building the template so the copied
context never shows "undefined".
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Show resolved
Hide resolved
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/migrations/20260131161904_add_ai_feedback_context_types.sql`:
- Around line 63-88: Add validation for the 'test_insights' context similar to
the other branches: when p_context_type = 'test_insights' verify that
public.assignments has a row with id = p_resource_id and class_id = p_class_id,
and return json_build_object('error', 'Assignment not found in this class') if
not found; update the conditional block that currently skips validation to
include this check using the existing p_context_type, p_resource_id, and
p_class_id variables (and reference public.assignments) so feedback cannot be
attached to an assignment from a different class.
When a user clicks the AI Help button without any active MCP tokens, show a setup dialog with step-by-step instructions for configuring Claude Desktop with the Pawtograder MCP server via mcp-proxy. https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
…ograder/platform into claude/pawtograder-mcp-server-J4xbx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@supabase/functions/_shared/MCPAuth.ts`:
- Around line 1-10: The file MCPAuth.ts fails the repo formatter; run the
project's formatter (e.g., npm run format or prettierrc/black used by the repo)
on supabase/functions/_shared/MCPAuth.ts to fix stylistic issues and commit the
changes. Ensure the file’s exports and symbols such as verifyApiToken,
mintSupabaseJwt, isTokenRevoked, and validateScopes remain unchanged
semantically after formatting and that the updated file passes the CI formatting
check.
In `@supabase/functions/mcp-server/index.ts`:
- Around line 1-16: The file's top comment block for the "Pawtograder MCP Server
Edge Function" is failing the repo formatter; run the project's formatter (e.g.,
npm/yarn run format or prettier --write) against this file (index.ts) to
reformat the header and entire file so it passes lint/format checks, then stage
the updated file for commit.
|
@coderabbitai fullreview |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@lib/edgeFunctions.ts`:
- Around line 393-401: The client wrappers (mcpTokensList, mcpTokensCreate,
mcpTokensRevoke) assume data.error is a GenericResponse error object but the
edge function can return a string; update each wrapper to normalize the error
before throwing: after invoking the edge function, inspect data.error and if
it's a string convert it to the expected shape (e.g. { message: string, details:
undefined, recoverable: false }) and then throw new
EdgeFunctionError(normalizedError); keep using FunctionTypes.GenericResponse for
the normal case and apply the same normalization logic in all three functions so
EdgeFunctionError always receives the correct object shape.
In `@supabase/functions/mcp-server/auth.ts`:
- Around line 49-54: The error handling assumes response.json() always succeeds;
if the OAuth endpoint returns non-JSON (HTML or empty body) the JSON parse will
throw and hide the real HTTP status — update the token exchange and refresh
logic (the block throwing in auth.ts that reads const error: OAuthError = await
response.json(), and the similar logic in refreshAccessToken) to defensively try
parsing JSON in a try/catch, and on parse failure fall back to reading text via
response.text() and include response.status, response.statusText and the raw
body text in the thrown Error message so callers see the real HTTP status and
body when JSON parsing fails.
In `@supabase/functions/mcp-server/data.ts`:
- Around line 29-51: The docstring and behavior diverge: update getSafeProfile
to enforce privacy by including is_private_profile in the select, check that
flag and return null when is_private_profile is true, and keep returning the
safe shape (id, name, avatar_url, class_id) when false; specifically modify the
Supabase query in getSafeProfile to select("id, name, avatar_url, class_id,
is_private_profile"), then after the query do if (data.is_private_profile)
return null (still omitting is_private_profile from the returned object), or
alternatively if you intend not to enforce privacy change the docstring to
remove the "Returns null if profile is private" sentence.
- Around line 318-340: The replies loop in the block that iterates repliesData
performs N+1 queries by calling getProfileName(supabase, reply.author) and
querying user_roles per reply; refactor to batch-fetch all required profile
names and user_roles before the loop (similar to the getHelpRequest fix):
collect all unique reply.author ids, call getProfileName in bulk or replace it
with a single profiles query, and query user_roles once with
.in("private_profile_id", [...ids]). Then build maps (authorId -> name, authorId
-> roleExists) and inside the for (const reply of repliesData) loop use those
maps to set author_name and is_staff and compute is_answer without additional DB
calls.
- Around line 699-731: The autograder.grader_repo.split("/") usage in this block
is fragile for malformed repo strings; validate the split result before using
[org, repo] (same fix as getHandoutFiles). Replace the direct destructure with a
check: split autograder.grader_repo into parts, ensure parts.length === 2
(non-empty org and repo), and if invalid either log/throw or skip processing
this autograder entry so you don't call github.listFilesInRepo with bad inputs;
update any error handling to make the failure explicit and reuse the same
validation logic you added for getHandoutFiles to locate this change.
- Around line 502-546: searchHelpRequests currently fetches a page of
help_request ids then calls the expensive getHelpRequest for each and
post-filters by options.assignmentId, which can return fewer than the requested
limit and causes N+1 overhead; either (A) change the initial supabase query to
join through submissions to filter help_requests by submission.assignment_id =
options.assignmentId (i.e. add a join/eq path so the DB returns only matching
help_request ids before calling getHelpRequest), referencing the "help_requests"
query and the options.assignmentId check, or (B) implement an over-fetch loop
inside searchHelpRequests that pages additional id results (using .limit/.range
or offset) and continues calling getHelpRequest until results.length >= limit
(or no more rows) to ensure you return up to the requested limit while avoiding
silent undercounting. Ensure the chosen approach still applies the status filter
and respects the existing limit semantics.
- Around line 472-497: The getSubmissionsForStudent function currently fetches
all submissions and calls getSubmission for each, causing query amplification;
modify getSubmissionsForStudent to accept an optional limit:number (default 10)
and an optional includeFiles:boolean (default false), apply .limit(limit) to the
Supabase query, and call getSubmission(supabase, sub.id, classId, true,
includeFiles) so bulk fetches omit file loading by default while still allowing
callers to request files when needed.
- Around line 632-672: Validate and normalize assignment.template_repo before
destructuring (handle full URLs, missing org, extra segments) and fail-fast or
skip if it doesn't resolve to exactly two parts (org and repo) so
github.listFilesInRepo is never called with malformed values; then cap the
number of files processed (e.g., MAX_FILES constant) and only iterate the first
N entries from github.listFilesInRepo, and replace the sequential await calls to
github.getFileFromRepo with a bounded-parallel approach (Promise.allSettled or a
small concurrency queue) to fetch file contents into codeFiles while still
applying excludePatterns and size checks; reference assignment.template_repo,
github.listFilesInRepo, github.getFileFromRepo, excludePatterns, and codeFiles
when making the changes.
- Around line 220-242: The loop over messagesData causes N+1 queries via
getProfileName and the user_roles query; instead, collect all unique profile_ids
from messagesData, then perform two batched queries: one against the profiles
table to get names (used by getProfileName replacement) and one against
user_roles filtered by classId and role IN ("instructor","grader") to get staff
flags; build lookup maps (profileId -> name, profileId -> isStaff) and then
iterate messagesData to push HelpRequestMessage entries using those maps. Update
references: replace per-iteration calls to getProfileName and the supabase
.from("user_roles") query with the batched lookups, keep messages, messagesData,
and classId identifiers.
- Around line 593-595: The .or() call interpolates options.searchQuery directly
(subject.ilike and body.ilike clauses) which allows PostgREST filter injection
and unintended wildcard matching; fix it by escaping backslashes and double
quotes in options.searchQuery, escape SQL wildcard characters (%) and (_) (e.g.,
prefix with backslash), then wrap the escaped value in double quotes before
building the .or() filter string so reserved PostgREST characters cannot break
out of the quoted literal; update the code path that constructs query.or(...)
(where options.searchQuery is used) to apply this escaping/quoting logic and use
the escaped value for both subject.ilike and body.ilike.
In `@supabase/functions/mcp-server/index.ts`:
- Around line 898-927: getSubmissionsForStudent currently translates a public
profile ID via getPrivateProfileId then retrieves up to limit submission ids and
calls getSubmission for each, causing an N+1 explosion; change it to return a
lightweight summary list by selecting only needed summary columns from the
submissions table (e.g., id, assignment_id, created_at, score/grade/status)
instead of calling getSubmission for each row, remove the
Promise.all(getSubmission(...)) call, and return the summarized rows (keep the
existing limit logic and privateProfileId check); leave detailed per-submission
fetching to getSubmission so callers can request full details when needed.
In `@supabase/migrations/20260130031926_create_api_tokens_table.sql`:
- Around line 71-76: The UPDATE policy on public.api_tokens ("Users can revoke
own tokens") is too permissive because it lets users modify token_id and others;
add a BEFORE UPDATE trigger function (public.prevent_api_token_mutation) that
compares NEW vs OLD and RAISEs an exception if token_id, scopes, user_id, or
expires_at are changed, allowing only revoked_at, name, and last_used_at to be
updated; then create a trigger (prevent_token_mutation) FOR EACH ROW BEFORE
UPDATE on public.api_tokens that executes this function so users cannot mutate
the immutable fields while keeping the existing RLS policy for ownership checks.
🧹 Nitpick comments (20)
components/discussion/ErrorPinModal.tsx (1)
255-260: Type assertion masks potentiallyundefinedvalue asnumber.On line 256,
(isClassLevel ? undefined : assignmentId) as numbercasts a potentiallyundefinedvalue tonumber. While this works because Supabase's client serializesundefinedtonullfor RPC params, theas numberassertion hides the optionality from TypeScript. Line 259 uses the same conditional pattern forp_class_idbut without the cast, suggesting the RPC type definition already acceptsundefinedthere.If the RPC type for
p_assignment_idtruly requiresnumber, this cast silently bypasses that contract. Consider updating the RPC type definition to acceptnumber | undefinedinstead, or use a consistent approach for both parameters.components/ai-help/AIHelpFeedbackPanel.tsx (2)
12-12: Duplicate type definition — consider importing fromlib/edgeFunctions.tsinstead.
AIHelpContextTypeis already defined identically inlib/edgeFunctions.ts(line 439). Maintaining two copies creates a divergence risk if one is updated without the other.♻️ Suggested fix
-export type AIHelpContextType = "help_request" | "discussion_thread" | "test_failure" | "build_error" | "test_insights"; +export type { AIHelpContextType } from "@/lib/edgeFunctions";
48-50: Silent error swallowing hides actionable diagnostics.The
catchblock discards the error entirely. While the boolean return is fine for the caller, consider logging the error (e.g.,console.error) so issues with the RPC don't disappear silently during development.supabase/functions/mcp-server/index.ts (1)
39-45: Wildcard CORS origin — acceptable for token-authenticated MCP, but worth documenting.
Access-Control-Allow-Origin: *is standard for MCP servers where auth is token-based rather than cookie-based. A brief comment noting this intentional choice would help future reviewers.supabase/migrations/20260131000000_ai_help_feedback_rpc.sql (1)
70-90: Exception handler swallows all errors — consider logging or returning the SQLSTATE.The
WHEN OTHERSblock returns a generic "Failed to save feedback" message. For aSECURITY DEFINERfunction, this is safe from a data-leak perspective, but it makes debugging insertion failures (constraint violations, type mismatches) quite painful. Consider at minimum includingSQLERRMin a debug-level log or returning the error code.supabase/functions/mcp-tokens/index.ts (3)
300-311: Error-to-status mapping relies on fragile string matching.Status codes are derived by matching substrings of
error.message. If anyone tweaks the error messages inauthenticateUserorassertUserIsInstructorOrGrader, the status mapping silently breaks and everything falls to 500.Consider using a custom error class with a
statusproperty instead.♻️ Suggested approach
+class HttpError extends Error { + status: number; + constructor(message: string, status: number) { + super(message); + this.status = status; + } +} // In authenticateUser: - throw new Error("Missing Authorization header"); + throw new HttpError("Missing Authorization header", 401); - throw new Error("Unauthorized"); + throw new HttpError("Unauthorized", 401); // In assertUserIsInstructorOrGrader: - throw new Error("MCP tokens are only available to instructors and graders"); + throw new HttpError("MCP tokens are only available to instructors and graders", 403); // In the catch block: - const status = - message === "Unauthorized" || message === "Missing Authorization header" - ? 401 - : message.includes("only available to") - ? 403 - : 500; + const status = error instanceof HttpError ? error.status : 500;
286-293: Malformed JSON body yields an unhelpful 500.If a POST or DELETE request has an invalid JSON body,
req.json()throws a generic parse error that the outer catch maps to a 500. A quick guard would improve the developer experience.♻️ Suggested approach
if (req.method === "POST") { - const body = await req.json(); + let body: CreateTokenRequest; + try { + body = await req.json(); + } catch { + return new Response(JSON.stringify({ error: "Invalid JSON body" }), { + status: 400, + headers: { ...corsHeaders, "Content-Type": "application/json" } + }); + } return await handlePost(authHeader, body); }
27-32: Wide-open CORS on a token management endpoint.
Access-Control-Allow-Origin: *is typical for Supabase edge functions since they rely on bearer tokens, but for a sensitive token-minting endpoint, consider restricting the origin to your known frontend domain if feasible.components/settings/MCPTokensMenu.tsx (1)
89-105: No confirmation before revoking a token.Revoking is irreversible. A misclick on the trash icon (Line 274) immediately destroys the token. Consider adding a confirmation step — even a simple
window.confirmor a popover — to prevent accidental revocations.lib/edgeFunctions.ts (2)
440-440:AIHelpContextTypeis defined in two places.This type is also exported from
components/ai-help/AIHelpFeedbackPanel.tsx(Line 11). Consider consolidating to a single source of truth to avoid drift.
460-471: RPC type bypass viaCallableFunctioncast.The comment acknowledges the generated types may not include this RPC yet. This is fine as a temporary measure, but worth tracking so it doesn't linger once types are regenerated.
components/ai-help/AIHelpSubmissionErrorButton.tsx (1)
126-146:useCallbackdependency onpropsobject defeats memoization.Since
propsis a new object reference on every render, this callback is recreated every time. Destructure the needed values into the dependency array instead.♻️ Proposed fix
+ const { errorType, testName, errorOutput, assignmentId, classId, submissionId } = props; + const handleCopy = useCallback(async () => { const prompt = generateErrorPrompt(props); // ... - }, [props]); + }, [errorType, testName, errorOutput, assignmentId, classId, submissionId]);You'd also need to pass the individual values or reconstruct the props object inside the callback.
lib/test-insights/AIHelpTestErrorButton.tsx (1)
29-29: Redundant slice —submissionIdsis sliced to 10, then only 3 are used.Line 29 slices to 10, but lines 61-62 re-slice to 3. The intermediate variable of 10 isn't used elsewhere. Simplify to
slice(0, 3)directly, or document the intent if 10 is planned for future use.components/ai-help/AIHelpButton.tsx (2)
537-606: Extract shared logic into a custom hook to reduce duplication.
AIHelpIconButtonduplicates the token-checkuseEffect(lines 550–566 vs 409–427), theuseMemoprompt (lines 568–578 vs 429–439), and the copy/feedback/setup-dialog state. A custom hook likeuseAIHelp(props)returning{ prompt, hasTokens, showSetupDialog, ... }would consolidate this nicely and ensure both variants stay in sync.Sketch of a shared hook
function useAIHelp(props: Omit<AIHelpButtonProps, "size" | "variant">) { const isInstructorOrGrader = useIsGraderOrInstructor(); const [showFeedback, setShowFeedback] = useState(false); const [showSetupDialog, setShowSetupDialog] = useState(false); const [hasTokens, setHasTokens] = useState<boolean | null>(null); useEffect(() => { if (!isInstructorOrGrader) return; async function checkTokens() { try { const supabase = createClient(); const { tokens } = await mcpTokensList(supabase); const now = new Date(); const hasActiveToken = tokens?.some( (t) => !t.revoked_at && new Date(t.expires_at) > now ); setHasTokens(hasActiveToken ?? false); } catch { setHasTokens(true); } } checkTokens(); }, [isInstructorOrGrader]); const prompt = useMemo(() => generateAIPrompt(props), [ props.contextType, props.resourceId, props.classId, props.assignmentId, props.submissionId, ]); const handleClose = useCallback(() => { setShowFeedback(false); setShowSetupDialog(false); }, []); return { isInstructorOrGrader, prompt, hasTokens, showFeedback, setShowFeedback, showSetupDialog, setShowSetupDialog, handleClose, }; }
478-486: Conditional renders replace the button entirely, causing potential layout shifts.When
showSetupDialogorshowFeedbackis true, the component returns only the dialog/panel — the button disappears from its position in the DOM. In a toolbar orHStackcontext, this causes a layout jump. Consider wrapping the button and the conditional panels together so the button remains while the dialog/panel overlays or appears adjacent to it.Sketch: keep button visible alongside panels
+ return ( + <> + <MCPSetupDialog isOpen={showSetupDialog} onClose={handleClose} /> + {showFeedback ? ( + <AIHelpFeedbackPanel classId={classId} contextType={contextType} resourceId={resourceId} onClose={handleClose} /> + ) : showContext ? ( + /* ... context box ... */ + ) : ( + <Tooltip content="Get AI assistance for helping this student" showArrow> + <Button size={size} variant={variant} colorPalette="purple" onClick={handleButtonClick}> + <Icon as={BsRobot} /> + AI Help + </Button> + </Tooltip> + )} + </> + );supabase/functions/_shared/MCPAuth.ts (1)
48-49: Unbounded JWT cache could grow in long-lived isolates.
supabaseJwtCacheis aMapwith no size limit or eviction. Within a warm Supabase Edge Function isolate, entries accumulate for every distinctuserIdthat makes an MCP request. While the 60s TTL means entries go stale quickly, stale entries are never removed — only their JWTs stop being reused.For a "chill" review: this is unlikely to be a problem in practice given typical edge function lifetimes, but if the service sees sustained traffic from many users, consider pruning expired entries periodically.
supabase/migrations/20260130031926_create_api_tokens_table.sql (1)
87-89:revoked_token_idsSELECT grant toauthenticatedmay be unnecessary.The MCP auth code (
MCPAuth.tsline 197) queriesrevoked_token_idsusing the service role key, bypassing RLS entirely. GrantingSELECTtoauthenticatedon a security-sensitive revocation table exposes token revocation status to any authenticated user. Unless there's a client-side need to read this table, consider removing this grant.supabase/functions/mcp-server/types.ts (2)
107-107:unknown | nullis redundant —unknownalready encompassesnull.Since
unknownis the top type in TypeScript, the| nulladds nothing. Consider simplifying to justunknown.Suggested fix
- errors: unknown | null; + errors: unknown;
141-150:HandoutFileContextandGraderFileContextare structurally identical.Both have
{ path: string; content: string }. If these are unlikely to diverge, a sharedFileContexttype alias could reduce duplication. That said, keeping them separate is defensible if you anticipate them evolving independently (e.g., addinglanguageto handout files ortest_frameworkto grader files).supabase/functions/mcp-server/data.ts (1)
612-678: Consider extracting shared repo-file-fetching logic.
getHandoutFilesandgetGraderFilesshare nearly identical file enumeration, filtering, and content-fetching logic (exclude patterns, size limit, error handling). Extracting a shared helper likefetchRepoFiles(repoSlug: string, excludePatterns: RegExp[]): Promise<FileContext[]>would eliminate ~40 lines of duplication and make both functions easier to maintain.Sketch
async function fetchRepoFiles( repoSlug: string, excludePatterns: RegExp[], maxFileSize = 100000 ): Promise<{ path: string; content: string }[]> { const parts = repoSlug.split("/"); if (parts.length !== 2 || !parts[0] || !parts[1]) { console.error(`Invalid repo format: ${repoSlug}`); return []; } const [org, repo] = parts; const files = await github.listFilesInRepo(org, repo); const results: { path: string; content: string }[] = []; for (const file of files) { if (excludePatterns.some((p) => p.test(file.path))) continue; if (file.size > maxFileSize) continue; try { const content = await github.getFileFromRepo(repoSlug, file.path); if (content && "content" in content) { results.push({ path: file.path, content: content.content }); } } catch { continue; } } return results; }Also applies to: 680-737
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@supabase/functions/mcp-server/data.ts`:
- Around line 679-685: The current searchDiscussionThreads implementation
creates an N+1 by calling getDiscussionThread for each item (which loads author
profile, assignment, latest submission, replies); instead, change
searchDiscussionThreads to build and return lightweight
DiscussionThreadContext-like summaries directly from the search query results
(select only id, title, author_id, created_at, reply_count, assignment_id,
last_reply_at, etc.), stop calling getDiscussionThread per thread, collect all
unique author_ids/reply_author_ids and batch-fetch profiles (similar to the
index.ts approach), join/map the fetched profiles into the summary objects, and
do not fetch nested submission data or full reply payloads in this path to avoid
the N+1 behavior.
- Around line 100-122: getSubmissionFiles currently issues a supabase query
without a row limit which can return unbounded results; update the query in
getSubmissionFiles to apply the same cap used elsewhere (e.g., .limit(MAX_ROWS))
and ensure MAX_ROWS is imported or defined in this module, then add the
.limit(MAX_ROWS) call before .order(...) (or after select) so the returned rows
are bounded and match the behavior in index.ts.
- Around line 1-27: The file data.ts is an unused duplicate of index.ts and
should be removed to avoid maintenance drift; delete
supabase/functions/mcp-server/data.ts and ensure the canonical
implementations/exporters for getAssignment, getSubmission, getSubmissionFiles,
getLatestSubmissionForStudent, getHelpRequest, getDiscussionThread,
searchHelpRequests, searchDiscussionThreads, getHandoutFiles, and getGraderFiles
remain in index.ts (verify index.ts exports these symbols) and run a repo-wide
search to confirm no imports/reference to data.ts remain before committing the
deletion.
- Around line 836-874: getGraderFiles currently iterates all files sequentially
with no cap or concurrency control, which can be slow and hit GitHub rate
limits; update getGraderFiles to mirror getHandoutFiles by enforcing MAX_FILES
(100) to limit how many files are processed and using FILE_FETCH_CONCURRENCY
(10) to bound concurrent calls to github.getFileFromRepo (e.g., use the same
PromisePool/p-limit pattern or helper used by getHandoutFiles) while preserving
the same excludePatterns, size check, and push into graderFiles (path/content)
and keep the same skip-on-error behavior.
In `@supabase/functions/mcp-server/index.ts`:
- Around line 1098-1104: The "notifications/initialized" branch currently
returns a JSON-RPC response even though notifications must not respond; update
the handler in handleMCPRequest so the case "notifications/initialized" returns
undefined/null (or simply falls through) instead of { jsonrpc: "2.0", id,
result: {} } — locate the switch/case for "notifications/initialized" and
remove/replace the explicit response with a no-op return to ensure no JSON-RPC
reply is emitted for that notification.
- Around line 978-1038: executeTool currently uses unchecked "as" casts for args
which can forward invalid types to Supabase; validate each tool's required
inputs before calling the implementation (use the TOOLS entry or an inputSchema
per tool) inside executeTool: check presence and runtime types (typeof for
string/boolean, Number.isFinite(Number(...)) for numbers), coerce/parse safely
when appropriate, and throw a descriptive error if a required field is missing
or has the wrong type; apply this validation for callers of getHelpRequest,
getDiscussionThread, getSubmission, getSubmissionsForStudent, getAssignment,
searchHelpRequests, and searchDiscussionThreads so only correctly typed values
are passed through.
- Around line 1213-1226: getEndpointUrl currently trusts x-forwarded-proto and
x-forwarded-host which allows header spoofing; update getEndpointUrl to avoid
using untrusted forwarded headers when EDGE_FUNCTIONS_URL is not set: either (a)
refuse the fallback by throwing/logging a clear warning and stop returning a
computed endpoint (so the SSE "endpoint" event cannot emit a spoofed URL), or
(b) derive the fallback only from the request's own URL origin (new
URL(req.url).origin) and explicitly ignore x-forwarded-proto/x-forwarded-host
unless a trusted-proxy flag or allowlist is present; change the function to
reference EDGE_FUNCTIONS_URL and getEndpointUrl and add the warning/exception
behavior so the code never emits an endpoint based on unvalidated x-forwarded-*
headers.
- Around line 1323-1325: The code calls const body = await req.json() without
handling malformed JSON; wrap that call in a try/catch around the req.json()
invocation, catch SyntaxError (or any thrown error) and return a JSON-RPC parse
error response object with code -32700 and id set to null (and appropriate
Content-Type header) instead of letting it bubble to the outer catch; update the
request handler where const body = await req.json() is used to short-circuit and
send this JSON-RPC error response when parsing fails.
- Around line 942-971: The getSubmissionsForStudent summary currently fabricates
a full GraderResultContext with hardcoded defaults (e.g., id: 0, lint_passed:
false, lint_output: "") which are misleading; change the function to return a
distinct lightweight summary type (e.g., SubmissionSummary) or ensure
grader_result only contains true available fields (score and max_score) and set
all other grader fields to null/undefined (not plausible defaults). Locate the
mapping in getSubmissionsForStudent where submissions.map constructs the return
object (and the use of scoreMap.get(sub.id)), replace the fake
GraderResultContext shape with either a minimal summary object or null for
absent fields, and update any types/signatures so callers expecting
SubmissionContext are adjusted to accept the new SubmissionSummary.
In `@supabase/migrations/20260130031926_create_api_tokens_table.sql`:
- Around line 28-53: The prevent_api_token_mutation trigger function currently
omits created_at from its immutability checks, allowing users to alter this
audit field; update the function (prevent_api_token_mutation) to add a check
similar to the others that raises an exception when NEW.created_at IS DISTINCT
FROM OLD.created_at so created_at is treated as immutable alongside token_id,
scopes, user_id, and expires_at.
- Around line 114-118: The current DELETE policy "Users can delete own tokens"
on public.api_tokens lets JWTs remain valid after row deletion because
MCPAuth.ts:authenticateMCPRequest only checks signature and revoked_token_ids;
remove this DELETE policy to force revocation-only (preferred) or implement a
BEFORE DELETE trigger (e.g., handle_api_token_deletion + on_api_token_deleted)
that inserts OLD.token_id into public.revoked_token_ids (ON CONFLICT DO NOTHING)
before deletion so the existing revoke check works; do not rely solely on
updating MCPAuth.ts to query api_tokens.
🧹 Nitpick comments (6)
supabase/migrations/20260130031926_create_api_tokens_table.sql (2)
63-67:revoked_token_idshas no RLS — any authenticated user can read all revoked token IDs.RLS is enabled on
api_tokensbut not onrevoked_token_ids, and theGRANT SELECT ... TO authenticatedon line 125 means every logged-in user can enumerate all revoked token IDs across the platform. The token IDs themselves are opaque JTI values (low PII risk), but if the intent is for only edge functions (running withservice_role) to query this table, you can either:
- Enable RLS and add a restrictive policy (deny all for
authenticated, letservice_rolebypass), or- Remove the
GRANT SELECTforauthenticatedentirely if only the service role needs it, or- At minimum, add a comment explaining why unrestricted read access is intentional.
Based on learnings, migrations implementing RLS should clearly document intended access boundaries, especially when a related table is left without RLS.
Option 1: Lock it down to service_role only
+ALTER TABLE public.revoked_token_ids ENABLE ROW LEVEL SECURITY; + +-- No SELECT policy for authenticated — only service_role (which bypasses RLS) can read GRANT SELECT, INSERT, UPDATE, DELETE ON public.api_tokens TO authenticated; -GRANT SELECT ON public.revoked_token_ids TO authenticated; +GRANT SELECT ON public.revoked_token_ids TO service_role;Also applies to: 125-125
6-20: Consider a periodic cleanup strategy for expired/revoked tokens.The
api_tokenstable will accumulate rows over time as tokens expire or get revoked. There's no automatic cleanup mechanism (e.g., apg_cronjob or application-level sweep). For now this is fine, but as usage grows, consider adding a scheduled job to purge rows whereexpires_at < NOW() - interval '30 days'(and correspondingrevoked_token_idsentries).supabase/functions/mcp-server/index.ts (2)
40-45: Wildcard CORS origin — consider restricting in production.
Access-Control-Allow-Origin: "*"allows any origin to call this endpoint. Since this MCP server handles sensitive student data behind auth tokens, a compromised or malicious site could make authenticated cross-origin requests if a token is leaked. Consider restricting to known origins or at least making it configurable via environment variable.
200-230: Unbounded in-memory profile cache with no eviction — potential memory leak in long-lived edge function.The global
profileCachemaps grow indefinitely with no size cap or periodic cleanup. In a Supabase Edge Function, the runtime can be reused across requests, so a busy deployment serving many classes and profiles could accumulate stale entries well beyond TTL (they're only lazily evicted on read).Consider adding a maximum cache size with LRU eviction, or periodically pruning expired entries.
supabase/functions/mcp-server/data.ts (2)
150-213: Sequential waterfall of DB calls ingetHelpRequest.The data flow is strictly sequential: submission → assignment → latestSubmission → messages → batch lookups. Several of these could be parallelized. For example, the messages query doesn't depend on the submission result, and assignment + latestSubmission could be fetched concurrently (as
index.tsalready does at lines 576–602 and 615–618 withPromise.all).Note that
index.tsalready has an optimized version ofgetHelpRequestwith parallel fetches. If thisdata.tsversion is still called, consider aligning it.
439-513: Sequential queries ingetSubmission— tests and build output could be fetched in parallel.After fetching the grader result,
getSubmissionsequentially fetches tests (line 456) then build output (line 479). These are independent and could run concurrently. Theindex.tsversion already parallelizes these withPromise.all(lines 476–488).Suggested fix
if (graderData) { - const tests: TestResultContext[] = []; - const { data: testsData } = await supabase - .from("grader_result_tests") - ... - - // ... process tests ... - - let buildOutput: BuildOutputContext | null = null; - const { data: outputData } = await supabase - .from("grader_result_output") - ... + const [testsResult, outputResult] = await Promise.all([ + supabase + .from("grader_result_tests") + .select("id, name, part, score, max_score, output, output_format, is_released") + .eq("grader_result_id", graderData.id) + .order("id", { ascending: true }), + supabase + .from("grader_result_output") + .select("stdout, stderr, combined_output, output_format") + .eq("grader_result_id", graderData.id) + .maybeSingle() + ]); + // ... process testsResult.data and outputResult.data ...
data.ts was an unused duplicate of index.ts. Canonical implementations for getAssignment, getSubmission, getSubmissionFiles, getLatestSubmissionForStudent, getHelpRequest, getDiscussionThread, searchHelpRequests, searchDiscussionThreads, getHandoutFilesFiltered, and getGraderFilesFiltered remain in index.ts. No imports referenced data.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
- List all available MCP tools upfront so AI can auto-fetch context - Require direct evidence from code/spec/tests to support diagnosis - Add verification checklist for staff to cross-check AI reasoning - Provide two response versions: Socratic (exploratory) and Direct (efficient) - Encourage feedback in Pawtograder to improve AI assistance https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
3 participants
Summary
This PR introduces AI assistance capabilities for instructors and graders to help students more effectively. It includes a new MCP (Model Context Protocol) server that provides AI assistants with structured access to student help requests, discussion threads, and submission data, plus UI components for launching AI assistance from the help queue and discussion threads.
Key Changes
Frontend Components
AIHelpButton component (
components/ai-help/AIHelpButton.tsx): New component with two variants:AIHelpButton: Full button with expandable context displayAIHelpIconButton: Compact icon-only version for toolbar integrationIntegration points:
components/help-queue/help-request-chat.tsx)app/course/[course_id]/discussion/[root_id]/page.tsx)Backend MCP Server
New MCP server package (
packages/mcp-server/): Provides AI assistants with secure, privacy-respecting access to:Authentication & Authorization:
Privacy Protection:
Assignment Form Enhancement
app/course/[course_id]/manage/assignments/new/form.tsx)Implementation Details
MCP Context Generation: The frontend generates structured MCP prompts that include:
Tool Availability: MCP server provides tools for:
get_help_request: Fetch help request with contextget_discussion_thread: Fetch discussion with repliesget_submission: Fetch submission with test resultsget_assignment: Fetch assignment detailssearch_help_requests/search_discussion_threads: Find related contentUser Experience: Instructors can click "AI Help" button to copy a pre-formatted prompt that can be pasted into Claude, ChatGPT, or any MCP-compatible AI assistant
Testing Recommendations
https://claude.ai/code/session_01Kdiup6E98XWAXB9UTA3qE4
Summary by CodeRabbit
New Features
UI Enhancements
Chores