diff --git a/CLAUDE.md b/CLAUDE.md index a0a7570..863a604 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,6 +123,20 @@ These cost real time to discover; `docs/API.md` has the full list with evidence. - **Never swallow a per-item crawl error.** Board failures used to be caught and dropped, so the index lost whole boards while the crawl reported success — which is how the 20-id limit went unnoticed. They go into `Snapshot.failures`. +- **`GET /lessons/{id}/tasks` is a bare array whose items carry no id.** Not the + `{data,total}` envelope, and `LessonLinkedTaskResponse` has no id field at + all. A topic-attached task is thus unidentifiable from the API and invisible + in both task lists once past due — 18 of 60 tasks on the real account. + `core/lesson-page.ts` scrapes the ids off the legacy topic page. +- **Collaborative text editor (Etherpad) contents are reachable, in two hops.** + `GET /api/v3/collaborative-text-editor/content-element/{id}` returns the pad + url *and* sets an Etherpad `sessionID` cookie; `/etherpad/p/{id}/export/txt` + then returns the text. No Etherpad API key needed. `core/etherpad.ts` checks + the url's host before sending the cookie to it. +- **A draft board is listed on the course page but 403s when opened.** Say "not + published yet", not "no access". +- **File records are mutable**: `PATCH /file/rename/{id}` keeps the id and size, + so the store's digest has to include the name. - **Submissions: only `GET /submissions/status/task/{taskId}` exists.** No list, no fetch-by-id, and the payload has no submitted text, grade comment or graded-at — `/api/v1`, which had them, is not served here. Don't imply absent diff --git a/docs/API.md b/docs/API.md index 8051b8c..e9b3fc2 100644 --- a/docs/API.md +++ b/docs/API.md @@ -173,6 +173,31 @@ clearest case in this API of live behaviour diverging from upstream source. instance configuration, including the session timeouts and feature flags. Handy for checking deployed settings without a token. +**`GET /lessons/{id}/tasks` returns a bare array, and its items have no id.** +Every other list endpoint returns `{data, total}`; this one returns the array +directly, so reading `.data` silently yields `undefined`. Worse, the items are +`LessonLinkedTaskResponse`, which has no id property at all — name, description +and dates only. A task attached to a topic is therefore unidentifiable from the +API: it is not a task element on the course page (the topic reports only +`numberOfPublishedTasks`), and once past due it is in neither `/tasks` nor +`/tasks/finished`. On the account this server was built for that hid 18 of 60 +tasks, submissions and grades included. The ids are recoverable only from the +legacy topic page, which links each task as `/homework/{id}` — +`core/lesson-page.ts`. + +**A student's task lists exclude past-due tasks.** `/tasks` drops a task once +its due date passes; `/tasks/finished` holds only what the student ticked off. +A submitted, graded, past-due task is in neither. Reach it through the course +page, or through its topic. + +**An unpublished board is listed but cannot be opened.** The course-board +projection reports a draft board with its title, while `GET /boards/{id}` +answers 403 for anyone who cannot edit it. Treat a 403 there as "probably not +published yet", not as an access problem. + +**`PATCH /file/rename/{fileRecordId}` mutates a file record in place.** The id +and size stay the same, so any change detection keyed on those alone misses it. + **`Content-Disposition` on downloads is malformed.** It comes back as `attachment;; filename="…"` — note the doubled semicolon — and the filename is percent-encoded inside the quotes. Parse defensively. @@ -183,9 +208,14 @@ From `ContentElementType` in `schulcloud-server`, all seen live except where noted: `richText`, `file`, `fileFolder`, `link`, `drawing`, `collaborativeTextEditor`, `externalTool`, `videoConference`, `h5p`, `deleted`. -Collaborative text editor contents are **not** retrievable through the API — -`GET /api/v3/collaborative-text-editor/{parentType}/{parentId}` returns a URL to -the Etherpad-style editor, not the document text. +Collaborative text editor elements come back with `content: {}` — no pad id, no +url, nothing. `GET /api/v3/collaborative-text-editor/content-element/{elementId}` +returns the pad url, and **also sets an Etherpad `sessionID` cookie** in its +response. With that cookie, Etherpad's own `/etherpad/p/{padId}/export/txt` +returns the document as plain text. So the contents *are* reachable, in two +hops and without Etherpad's API key; `core/etherpad.ts` does this. The url is +built from the server's `ETHERPAD__PAD_URI`, so it must be checked against the +instance host before the session cookie is sent to it. ## Re-verifying after an upstream release diff --git a/scripts/smoke.mjs b/scripts/smoke.mjs index b2b55db..ba1bc7b 100644 --- a/scripts/smoke.mjs +++ b/scripts/smoke.mjs @@ -106,30 +106,76 @@ const taskId = tasks.text.match(/\(`([0-9a-f]{24})`\)/)?.[1]; check('list_tasks finished', !(await call('list_tasks', { scope: 'finished' })).isError); check('list_news', !(await call('list_news')).isError); -// Walk courses until we find one with a board, to exercise the whole chain. +// Walk courses collecting boards and lessons, to exercise the whole chain. +// Every board id is collected rather than the first one taken: an unpublished +// board is listed on the course page with its title but 403s when opened, so +// "the first board in the course" is not reliably one that can be read. let boardId, fileId, lessonId, courseWithBoard; +const boardIds = []; +const topicsWithTasks = []; for (const id of courseIds) { const course = await call('get_course', { courseId: id }); if (course.isError) continue; courseWithBoard ??= id; - const b = course.text.match(/### Boards[\s\S]*?\(`([0-9a-f]{24})`\)/)?.[1]; - const l = course.text.match(/### Topics[\s\S]*?\(`([0-9a-f]{24})`\)/)?.[1]; - lessonId ??= l; - if (b && !boardId) boardId = b; - if (boardId && lessonId) break; + const boardsSection = course.text.match(/### Boards[\s\S]*?(?=\n### |$)/)?.[0] ?? ''; + for (const m of boardsSection.matchAll(/\(`([0-9a-f]{24})`\)/g)) boardIds.push(m[1]); + lessonId ??= course.text.match(/### Topics[\s\S]*?\(`([0-9a-f]{24})`\)/)?.[1]; + // A topic that reports tasks is the interesting one: those tasks are not task + // elements on the course page and carry no id in the API. + const topics = course.text.match(/### Topics[\s\S]*?(?=\n### |$)/)?.[0] ?? ''; + for (const m of topics.matchAll(/\(`([0-9a-f]{24})`\) — (\d+) task/g)) topicsWithTasks.push(m[1]); } check('get_course', Boolean(courseWithBoard), `first usable course ${courseWithBoard}`); -check('found a column board', Boolean(boardId), boardId); +check('found a column board', boardIds.length > 0, `${boardIds.length} board(s)`); -if (boardId) { - const board = await call('get_board', { boardId }); - check('get_board', !board.isError && /Board id:/.test(board.text)); +let board, drafts = 0; +for (const id of boardIds) { + const attempt = await call('get_board', { boardId: id }); + if (!attempt.isError) { + board = attempt; + boardId = id; + break; + } + if (/draft/i.test(attempt.text)) drafts++; +} +if (boardIds.length > 0) { + check( + 'get_board', + Boolean(board) && /Board id:/.test(board.text), + boardId ? `opened ${boardId}${drafts ? `, skipped ${drafts} unpublished` : ''}` : 'no board could be opened', + ); +} +if (board) { + // Pads carry real content and the board API returns them empty, so the text + // comes from Etherpad itself. Either it was read, or the tool says plainly + // that it was not — it must never claim the contents cannot be had. + const padLine = board.text.match(/- Collaborative text document `[0-9a-f]{24}`[^\n]*/)?.[0]; + check( + 'collaborative text documents report contents or say they are empty', + padLine === undefined || /:$|\(empty, or its contents could not be read\)/.test(padLine), + padLine ?? 'no pad on this board', + ); fileId = board.text.match(/File: \*\*[^*]+\*\* \(`([0-9a-f]{24})`/)?.[1]; check('get_board resolved attachments', Boolean(fileId), fileId ?? 'no files on this board'); check('get_board includeFiles=false', !(await call('get_board', { boardId, includeFiles: false })).isError); } if (lessonId) check('get_lesson', !(await call('get_lesson', { lessonId })).isError, lessonId); + +// A task attached to a topic is reachable only if its id was recovered from the +// topic page: the API's topic-task projection has no id field, and such a task +// is on no course page and drops out of both task lists once it is past due. +if (topicsWithTasks.length > 0) { + const lesson = await call('get_lesson', { lessonId: topicsWithTasks[0] }); + const topicTaskId = lesson.text.match(/### Tasks in this lesson[\s\S]*?\(`([0-9a-f]{24})`\)/)?.[1]; + check('get_lesson lists a topic\'s tasks with ids', Boolean(topicTaskId), topicTaskId ?? lesson.text.slice(0, 90)); + if (topicTaskId) { + const viaTopic = await call('get_task', { taskId: topicTaskId }); + check('get_task opens a task found only through a topic', !viaTopic.isError && /Task id:/.test(viaTopic.text)); + } +} else { + check('get_lesson lists a topic\'s tasks with ids', true, 'no topic on this account reports tasks — nothing to check'); +} if (taskId) { const task = await call('get_task', { taskId }); check('get_task', !task.isError && /Task id:/.test(task.text), taskId); @@ -180,13 +226,25 @@ check( hasIndex ? !status.isError : status.isError && /not configured/.test(status.text), status.text.split('\n')[0], ); -const changed = await call('what_changed', { since: '2026-01-01' }); -check('what_changed responds', hasIndex ? !changed.isError : changed.isError); if (hasIndex) { + // Populate before asking what changed: a brand-new index holds no generations + // to diff, and what_changed rightly refuses rather than inventing a baseline. const refreshed = await call('refresh_index', { courseId: courseIds[0], force: true }); check('refresh_index re-crawls one course', !refreshed.isError, refreshed.text.split('\n')[0]); +} +const changed = await call('what_changed', { since: '2026-01-01' }); +check('what_changed responds', hasIndex ? !changed.isError : changed.isError, changed.text.split('\n')[0]); +if (hasIndex) { + // Both the hit and the no-hit answer say when the index was last refreshed; + // a live crawl (fresh=true) says nothing of the sort. That is what separates + // "answered from the index" from "answered by crawling", whatever the term + // happens to match in this account's data. const indexed = await call('search', { query: searchTerm }); - check('search uses the index and states freshness', !indexed.isError && /Index /.test(indexed.text)); + check( + 'search uses the index and states freshness', + !indexed.isError && /refreshed/i.test(indexed.text), + indexed.text.split('\n')[0], + ); } console.log('\n== api_get guard rails =='); diff --git a/src/core/board.ts b/src/core/board.ts index 22cc4c8..7e418af 100644 --- a/src/core/board.ts +++ b/src/core/board.ts @@ -1,4 +1,6 @@ +import type { Config } from '../config.ts'; import type { SchulcloudClient } from './client.ts'; +import { fetchPadText } from './etherpad.ts'; import { SchulcloudApiError } from './client.ts'; import type { BoardSkeleton, CardResponse, ContentElement, FileRecord } from './types.ts'; @@ -22,6 +24,8 @@ export interface AssembledElement { files: FileRecord[]; /** Set when this element's files could not be resolved. */ fileError?: string; + /** What a class actually wrote in a collaborativeTextEditor (Etherpad) pad. */ + padText?: string; raw: Record; } @@ -52,7 +56,7 @@ export async function assembleBoard( client: SchulcloudClient, boardId: string, schoolId: string, - options: { resolveFiles?: boolean } = {}, + options: { resolveFiles?: boolean; resolvePads?: Config } = {}, ): Promise { const resolveFiles = options.resolveFiles ?? true; @@ -71,6 +75,13 @@ export async function assembleBoard( await attachFiles(client, assembled, schoolId); } + // A pad's text is real course content, and nothing else surfaces it: the + // board API returns collaborativeTextEditor elements with empty content. + // Costs two requests per pad and only when a board has one. + if (options.resolvePads) { + await attachPadText(options.resolvePads, assembled); + } + const fileCount = assembled .flatMap((column) => column.cards) .flatMap((card) => card.elements) @@ -121,6 +132,29 @@ function buildElement(element: ContentElement): AssembledElement { return assembled; } +/** + * Fills in the text of each collaborative text editor element. + * + * Failures are left silent rather than recorded: unlike a missing attachment, + * an unreadable pad is usually an empty one, and the element itself is still + * reported. + */ +async function attachPadText(config: Config, columns: AssembledColumn[]): Promise { + const pads = columns + .flatMap((column) => column.cards) + .flatMap((card) => card.elements) + .filter((element) => element.type === 'collaborativeTextEditor'); + + await Promise.all( + pads.map(async (element) => { + const text = await fetchPadText(config, element.id); + if (!text) return; + // The element's own title, when it has one, stays as the heading. + element.padText = text; + }), + ); +} + /** * Resolves file-bearing elements to file records. * diff --git a/src/core/client.ts b/src/core/client.ts index 897262e..ccddec4 100644 --- a/src/core/client.ts +++ b/src/core/client.ts @@ -13,6 +13,7 @@ import type { NewsResponse, Paginated, SubmissionStatus, + LessonLinkedTask, TaskContent, } from './types.ts'; @@ -304,8 +305,22 @@ export class SchulcloudClient { return this.getJson(`/api/v3/lessons/${encodeURIComponent(lessonId)}`); } - getLessonTasks(lessonId: string): Promise> { - return this.getJson>(`/api/v3/lessons/${encodeURIComponent(lessonId)}/tasks`); + /** + * A lesson's tasks. + * + * Returns a bare array, not the `{data, total}` envelope every other list + * endpoint uses — checked against both the live instance and a local 33.40. + * Typing it as `Paginated` made `.data` undefined, which silently dropped + * every task attached to a topic: they vanished from get_lesson, get_task + * reported them as non-existent, and their submissions — grades included — + * could not be reached at all. The envelope branch is kept in case the + * endpoint is ever normalised to match its siblings. + */ + async getLessonTasks(lessonId: string): Promise { + const body = await this.getJson>( + `/api/v3/lessons/${encodeURIComponent(lessonId)}/tasks`, + ); + return Array.isArray(body) ? body : (body.data ?? []); } // --- column boards --------------------------------------------------- diff --git a/src/core/crawl.ts b/src/core/crawl.ts index 600bc85..43d20f2 100644 --- a/src/core/crawl.ts +++ b/src/core/crawl.ts @@ -1,3 +1,4 @@ +import type { Config } from '../config.ts'; import { assembleBoard, type AssembledBoard } from './board.ts'; import type { SchulcloudClient } from './client.ts'; import { htmlToText, normalizeObjectId } from './text.ts'; @@ -90,6 +91,11 @@ export interface CrawlOptions { includeLessonContents?: boolean; /** Resolve board file elements to file records. */ includeFiles?: boolean; + /** + * Read the text of collaborative text editor (Etherpad) pads, which needs a + * second credentialled hop outside the API. Omit to leave pads unread. + */ + config?: Config; courseConcurrency?: number; boardConcurrency?: number; onProgress?: (done: number, total: number, label: string) => void; @@ -203,7 +209,10 @@ async function crawlCourse( await forEachLimited(boardIds, options.boardConcurrency ?? 4, async (boardId) => { let assembled: AssembledBoard; try { - assembled = await assembleBoard(client, boardId, options.schoolId, { resolveFiles: includeFiles }); + assembled = await assembleBoard(client, boardId, options.schoolId, { + resolveFiles: includeFiles, + resolvePads: options.config, + }); } catch (error) { // Record rather than swallow: a dropped board used to disappear from the // index while the crawl still reported success, which is how a 20-card @@ -222,6 +231,9 @@ async function crawlCourse( parts.push(card.title); for (const element of card.elements) { if (element.text) parts.push(htmlToText(element.text)); + // Pad contents are course material like any other; without this they + // are unsearchable, and a pad is often where the actual group work is. + if (element.padText) parts.push(element.padText); if (element.url) parts.push(element.url); for (const record of element.files) { files.push({ diff --git a/src/core/etherpad.ts b/src/core/etherpad.ts new file mode 100644 index 0000000..4b8a49b --- /dev/null +++ b/src/core/etherpad.ts @@ -0,0 +1,100 @@ +import type { Config } from '../config.ts'; + +/** + * Reads the text of a collaborative text editor (Etherpad) element. + * + * The board API is no help on its own: a `collaborativeTextEditor` element + * comes back with `content: {}` — no pad id, no url, nothing. What a class + * actually wrote in it is invisible to every other tool here. + * + * Two calls recover it, and neither needs Etherpad's own API key (which is a + * server-side secret this process has no business holding): + * + * 1. `GET /api/v3/collaborative-text-editor/content-element/{id}` answers with + * the pad url *and*, in a `Set-Cookie`, an Etherpad `sessionID` — the same + * exchange the web client performs before it embeds the pad. + * 2. Etherpad's own `/p/{padId}/export/txt` returns the pad as plain text to + * whoever holds that session. + * + * The session cookie is only ever sent back to the instance's own host: step 1 + * returns a url built from the server's `ETHERPAD__PAD_URI`, and a value + * pointing anywhere else is refused rather than followed. + * + * Everything degrades to `undefined`. A pad that cannot be read costs its text, + * never the board. + */ +export async function fetchPadText(config: Config, elementId: string): Promise { + try { + const handle = await fetchPadHandle(config, elementId); + if (!handle) return undefined; + + const response = await fetch(`${handle.origin}/etherpad/p/${handle.padId}/export/txt`, { + headers: { Cookie: handle.sessionCookie, Accept: 'text/plain' }, + signal: AbortSignal.timeout(config.requestTimeoutMs), + }); + if (!response.ok) return undefined; + + const text = (await response.text()).trim(); + // A pad nobody has typed in still exports the placeholder the instance + // seeds new pads with; reporting that as content would be a lie. + return text.length > 0 && text !== DEFAULT_PAD_TEXT ? text : undefined; + } catch { + return undefined; + } +} + +/** The instance's `DEFAULT_PAD_TEXT`; an untouched pad exports exactly this. */ +const DEFAULT_PAD_TEXT = 'Schreib etwas!'; + +interface PadHandle { + origin: string; + padId: string; + sessionCookie: string; +} + +async function fetchPadHandle(config: Config, elementId: string): Promise { + const response = await fetch( + `${config.baseUrl}/api/v3/collaborative-text-editor/content-element/${encodeURIComponent(elementId)}`, + { + headers: { Authorization: `Bearer ${config.jwt}`, Accept: 'application/json' }, + signal: AbortSignal.timeout(config.requestTimeoutMs), + }, + ); + if (!response.ok) return undefined; + + const body = (await response.json()) as { url?: unknown }; + if (typeof body.url !== 'string') return undefined; + + const padId = padIdFromUrl(body.url, config.baseUrl); + if (!padId) return undefined; + + // `getSetCookie` keeps the header split correctly; a plain `get` would join + // several cookies on the commas that appear inside the session list itself. + const sessionCookie = response.headers + .getSetCookie() + .map((cookie) => /^(sessionID=[^;]*)/.exec(cookie)?.[1]) + .find((value): value is string => Boolean(value)); + if (!sessionCookie) return undefined; + + return { origin: new URL(config.baseUrl).origin, padId, sessionCookie }; +} + +/** + * The pad id out of the url the server hands back. + * + * Refuses a url on another host: that url is server-configured, and following + * it blindly would send an Etherpad session cookie wherever it pointed. + * Group pad ids contain `$`, so the segment is kept exactly as encoded. + */ +export function padIdFromUrl(url: string, baseUrl: string): string | undefined { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + return undefined; + } + if (parsed.host !== new URL(baseUrl).host) return undefined; + + const segment = /\/etherpad\/p\/([^/?#]+)/.exec(parsed.pathname)?.[1]; + return segment && segment.length > 0 ? segment : undefined; +} diff --git a/src/core/lesson-page.ts b/src/core/lesson-page.ts new file mode 100644 index 0000000..fd32ab1 --- /dev/null +++ b/src/core/lesson-page.ts @@ -0,0 +1,84 @@ +import type { Config } from '../config.ts'; +import { decodeEntities } from './text.ts'; +import type { LessonLinkedTask } from './types.ts'; + +/** + * Recovers the ids of tasks that hang off a topic ("Thema"). + * + * `GET /api/v3/lessons/{id}/tasks` returns the topic's tasks — with their name, + * description and dates, but **no id**: `LessonLinkedTaskResponse` has no id + * field at all, by design rather than by omission. The course page is no help + * either, because a topic-attached task is not a task element there; the topic + * reports only `numberOfPublishedTasks`. + * + * The consequence is that those tasks were unreachable: absent from both task + * lists once past due (open excludes them, finished only holds what the student + * ticked off), absent from the course page, and unidentifiable from the topic. + * Their submissions, and so their grades, could not be read at all. On the + * account this was written for that is 18 of 60 tasks. + * + * The legacy topic page links each task as `/homework/{id}`, so it carries the + * mapping the API withholds. Like the homework-page scrape this authenticates + * by `jwt` **cookie**, hangs off an accessibility attribute rather than + * presentation markup, and degrades to an empty list — a markup change costs + * the ids again, never an error. + */ + +export interface LessonTaskLink { + id: string; + name: string; +} + +export async function fetchLessonTaskLinks( + config: Config, + courseId: string, + lessonId: string, +): Promise { + const url = `${config.baseUrl}/courses/${encodeURIComponent(courseId)}/topics/${encodeURIComponent(lessonId)}`; + try { + const response = await fetch(url, { + headers: { Cookie: `jwt=${config.jwt}`, Accept: 'text/html' }, + signal: AbortSignal.timeout(config.requestTimeoutMs), + redirect: 'follow', + }); + // Redirected away means the cookie was not accepted; there is nothing to + // parse and nothing worth raising. + if (!response.ok || !new URL(response.url).hostname.endsWith(new URL(config.baseUrl).hostname)) { + return []; + } + return parseLessonTaskLinks(await response.text()); + } catch { + return []; + } +} + +/** Exported for testing: the parsing is pure and deserves fixtures, not a network. */ +export function parseLessonTaskLinks(html: string): LessonTaskLink[] { + // `aria-label="Details der Aufgabe: 'name'"` exists for screen readers, which + // makes it far steadier than the surrounding layout. + const pattern = /]*aria-label="[^"']*'([^']*)'/g; + const found: LessonTaskLink[] = []; + const seen = new Set(); + for (const match of html.matchAll(pattern)) { + const id = match[1] ?? ''; + if (seen.has(id)) continue; + seen.add(id); + found.push({ id, name: decodeEntities(match[2] ?? '').trim() }); + } + return found; +} + +/** + * Pairs the API's id-less task bodies with the ids scraped from the topic page. + * + * Matching is by name, which is what both sides agree on. A name the page does + * not account for yields a task without an id: still worth reporting (it is + * visible to the user), just not something the id-taking tools can open. + */ +export function withScrapedIds(tasks: LessonLinkedTask[], links: LessonTaskLink[]): LessonLinkedTask[] { + const byName = new Map(links.map((link) => [link.name, link.id])); + return tasks.map((task) => { + const id = task.id ?? byName.get(task.name?.trim() ?? ''); + return id ? { ...task, id } : task; + }); +} diff --git a/src/core/types.ts b/src/core/types.ts index 1624e06..0506bd2 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -71,6 +71,39 @@ export interface TaskContent { status: TaskStatus; } +/** + * A topic's task as `GET /lessons/{id}/tasks` returns it. + * + * Deliberately not a `TaskContent`: the response carries no `id` and no + * `status` — `LessonLinkedTaskResponse` simply has no id property. Treating it + * as a TaskContent made `task.id` undefined at runtime while the type claimed + * otherwise, which is how topic-attached tasks went missing in silence. + * `core/lesson-page.ts` recovers the ids. + */ +export interface LessonLinkedTask { + /** Absent from the API; filled in from the topic page when it can be. */ + id?: string; + name: string; + description?: string; + availableDate?: string; + dueDate?: string | null; + courseId?: string; + courseName?: string; + lessonName?: string; + private?: boolean; + submissionIds?: string[]; + finishedIds?: string[]; +} + +/** + * A task as the tools render it, from whichever route found it. + * + * The task lists and course pages carry a `status`; the topic projection does + * not, and carries no id until one is scraped. Every `TaskContent` satisfies + * this, so list-derived tasks keep their full detail. + */ +export type ResolvedTask = LessonLinkedTask & { status?: TaskStatus }; + export interface LessonMetaContent { id: string; name: string; diff --git a/src/indexer/indexer.ts b/src/indexer/indexer.ts index 102e18c..21b1aab 100644 --- a/src/indexer/indexer.ts +++ b/src/indexer/indexer.ts @@ -113,6 +113,7 @@ export class Indexer { courseIds: scope === 'full' ? undefined : [scope], includeLessonContents: true, includeFiles: true, + config: this.config, }); const crawlId = await this.store.saveSnapshot(snapshot, scope); diff --git a/src/mcp/tools/content.ts b/src/mcp/tools/content.ts index 55d87bc..3d2a892 100644 --- a/src/mcp/tools/content.ts +++ b/src/mcp/tools/content.ts @@ -1,11 +1,19 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { z } from 'zod'; import type { ServerContext } from '../../context.ts'; +import { SchulcloudApiError } from '../../core/client.ts'; import { formatBytes } from '../../core/extract.ts'; import { dueLabel, formatDate, heading, htmlToText, joinSections, normalizeObjectId } from '../../core/text.ts'; import { assembleBoard, type AssembledBoard, type AssembledElement } from '../../core/board.ts'; import { forEachLimited } from '../../core/crawl.ts'; -import type { CourseBoardResponse, FileRecord, LessonResponse, TaskContent } from '../../core/types.ts'; +import { fetchLessonTaskLinks, withScrapedIds } from '../../core/lesson-page.ts'; +import type { + CourseBoardResponse, + FileRecord, + LessonLinkedTask, + LessonResponse, + ResolvedTask, +} from '../../core/types.ts'; import { failure, text, toToolError } from './result.ts'; import { describeSubmission } from './submissions.ts'; @@ -55,9 +63,24 @@ export function registerContentTools(server: McpServer, context: ServerContext): async ({ boardId, includeFiles }) => { try { const schoolId = await context.schoolId(); - const board = await assembleBoard(context.client, boardId, schoolId, { resolveFiles: includeFiles }); + const board = await assembleBoard(context.client, boardId, schoolId, { + resolveFiles: includeFiles, + resolvePads: context.config, + }); return text(formatBoard(board, includeFiles)); } catch (error) { + // An unpublished board 403s, while the course page lists its title + // regardless — the course-board projection does not filter drafts. + // Reporting that as "no permission" sends the reader looking for an + // access problem that does not exist; a draft is the common cause. + if (error instanceof SchulcloudApiError && error.status === 403) { + return failure( + `Board ${boardId} could not be opened (HTTP 403).\n\n` + + `The usual reason is that it is still a draft: an unpublished board is listed on ` + + `the course page with its title, but stays closed until the teacher publishes it. ` + + `Otherwise this account genuinely has no access to it.`, + ); + } return toToolError(error, `read board ${boardId}`); } }, @@ -80,12 +103,18 @@ export function registerContentTools(server: McpServer, context: ServerContext): const schoolId = await context.schoolId(); const [lesson, tasks, files] = await Promise.all([ context.client.getLesson(lessonId), - context.client.getLessonTasks(lessonId).catch(() => undefined), + context.client.getLessonTasks(lessonId).catch(() => []), context.client .listFiles({ storageLocationId: schoolId, parentType: 'lessons', parentId: lessonId }) .catch(() => undefined), ]); - return text(formatLesson(lesson, tasks?.data ?? [], files?.data ?? [])); + // The task bodies carry no id, so get_task cannot be pointed at them + // without the topic page. Only paid for when the topic has tasks. + const withIds = + tasks.length > 0 + ? withScrapedIds(tasks, await fetchLessonTaskLinks(context.config, lesson.courseId, lessonId)) + : tasks; + return text(formatLesson(lesson, withIds, files?.data ?? [])); } catch (error) { return toToolError(error, `read lesson ${lessonId}`); } @@ -142,7 +171,7 @@ export function registerContentTools(server: McpServer, context: ServerContext): * to learn which course the task belongs to (unless told), then read the * description off that course's page. */ -async function findTask(context: ServerContext, taskId: string, courseId?: string): Promise { +async function findTask(context: ServerContext, taskId: string, courseId?: string): Promise { if (courseId) { const fromCourse = await taskFromCourse(context, courseId, taskId); if (fromCourse) return fromCourse; @@ -168,7 +197,7 @@ async function findTask(context: ServerContext, taskId: string, courseId?: strin // back to scanning course pages costs ~26 requests and a few seconds, which // is a fair price for the tool working instead of claiming the id is wrong. const courses = await context.client.listAllCourses().catch(() => []); - let found: TaskContent | undefined; + let found: ResolvedTask | undefined; await forEachLimited(courses, 6, async (course) => { if (found) return; const fromCourse = await taskFromCourse(context, course.id, taskId); @@ -181,7 +210,7 @@ async function taskFromCourse( context: ServerContext, courseId: string, taskId: string, -): Promise { +): Promise { const board = await context.client.getCourseBoard(courseId).catch(() => undefined); if (!board) return undefined; for (const element of board.elements) { @@ -189,6 +218,21 @@ async function taskFromCourse( return { ...element.content, courseId, courseName: element.content.courseName ?? board.title }; } } + + // A task can hang off a topic rather than the course page, and those are not + // listed as task elements — only as a count on the topic. Without this the + // task is unreachable: not in the lists (a submitted, past-due task is in + // neither open nor finished) and not on the course page either. + for (const element of board.elements) { + if (element.type !== 'lesson' || !element.content.numberOfPublishedTasks) continue; + const links = await fetchLessonTaskLinks(context.config, courseId, element.content.id); + if (!links.some((link) => link.id === taskId)) continue; + const tasks = await context.client.getLessonTasks(element.content.id).catch(() => []); + const match = withScrapedIds(tasks, links).find((task) => task.id === taskId); + if (match) { + return { ...match, courseId, courseName: match.courseName ?? board.title, lessonName: element.content.name }; + } + } return undefined; } @@ -252,6 +296,14 @@ function formatBoard(board: AssembledBoard, includeFiles: boolean): string { ]); } +/** Indents a pad's body so it reads as quoted content, not as board structure. */ +function indent(body: string): string { + return body + .split('\n') + .map((line) => ` > ${line}`.trimEnd()) + .join('\n'); +} + function formatElement(element: AssembledElement, includeFiles: boolean): string { switch (element.type) { case 'richText': { @@ -271,8 +323,16 @@ function formatElement(element: AssembledElement, includeFiles: boolean): string if (element.files.length === 0) return `- ${element.type} element \`${element.id}\` — no files${caption}`; return element.files.map((file) => `- ${formatFileLine(file)}${caption}`).join('\n'); } - case 'collaborativeTextEditor': - return `- Collaborative text document \`${element.id}\`${element.text ? ` — ${element.text}` : ''} (contents not available through the API)`; + case 'collaborativeTextEditor': { + const title = element.text ? ` — ${element.text}` : ''; + // The board API returns these with empty content; the text comes from + // the pad itself (core/etherpad.ts). Absent means empty or unreadable, + // which for a pad is usually "nobody has written in it yet". + if (!element.padText) { + return `- Collaborative text document \`${element.id}\`${title} (empty, or its contents could not be read)`; + } + return [`- Collaborative text document \`${element.id}\`${title}:`, indent(element.padText)].join('\n'); + } case 'externalTool': return `- External tool${element.text ? `: ${element.text}` : ''} \`${element.id}\``; case 'videoConference': @@ -292,7 +352,7 @@ export function formatFileLine(file: FileRecord): string { return `File: **${file.name}** (\`${file.id}\`, ${file.mimeType}, ${formatBytes(file.size)})${blocked}${pending}`; } -function formatLesson(lesson: LessonResponse, tasks: TaskContent[], files: FileRecord[]): string { +function formatLesson(lesson: LessonResponse, tasks: LessonLinkedTask[], files: FileRecord[]): string { const sections = (lesson.contents ?? []).map((entry) => { const title = entry.title?.trim(); const component = entry.component ?? 'unknown'; @@ -316,7 +376,12 @@ function formatLesson(lesson: LessonResponse, tasks: TaskContent[], files: FileR tasks.length > 0 && joinSections([ heading(3, `Tasks in this lesson (${tasks.length})`), - tasks.map((task) => `- **${task.name}** (\`${task.id}\`) — ${dueLabel(task.dueDate)}`).join('\n'), + tasks + .map((task) => { + const id = task.id ? ` (\`${task.id}\`)` : ''; + return `- **${task.name}**${id} — ${dueLabel(task.dueDate)}`; + }) + .join('\n'), ]), ]); } @@ -336,7 +401,7 @@ function formatLessonComponent(component: string, content: Record 0 ? ', graded' : ''}`, + // Absent for a task found through a topic: that projection reports no + // counts. The submission section below carries the authoritative state. + task.status + ? `- Submitted: ${task.status.submitted}/${task.status.maxSubmissions}${task.status.graded > 0 ? ', graded' : ''}` + : undefined, ] .filter(Boolean) .join('\n'), diff --git a/src/mcp/tools/search.ts b/src/mcp/tools/search.ts index 18701ca..3962b14 100644 --- a/src/mcp/tools/search.ts +++ b/src/mcp/tools/search.ts @@ -110,6 +110,9 @@ async function liveSearch( courseIds: courseId ? [courseId] : undefined, includeLessonContents: true, includeFiles: scoped, + // Same trade as files: worth two extra requests per pad when the caller + // named a course, too slow to do across every course they can see. + config: scoped ? context.config : undefined, }); const hits = searchSnapshot(snapshot, query, limit); diff --git a/src/mcp/tools/submissions.ts b/src/mcp/tools/submissions.ts index 611fef8..9657838 100644 --- a/src/mcp/tools/submissions.ts +++ b/src/mcp/tools/submissions.ts @@ -1,10 +1,11 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { z } from 'zod'; import type { ServerContext } from '../../context.ts'; +import { fetchLessonTaskLinks, withScrapedIds } from '../../core/lesson-page.ts'; import { forEachLimited } from '../../core/crawl.ts'; import { fetchSubmissionDetail } from '../../core/homework-page.ts'; import { dueLabel, heading, joinSections } from '../../core/text.ts'; -import type { FileRecord, SubmissionStatus, TaskContent } from '../../core/types.ts'; +import type { FileRecord, ResolvedTask, SubmissionStatus } from '../../core/types.ts'; import { formatFileLine } from './content.ts'; import { text, toToolError } from './result.ts'; @@ -52,7 +53,7 @@ export function registerSubmissionTools(server: McpServer, context: ServerContex const [me, tasks] = await Promise.all([context.me(), collectTasks(context, scope, courseId, limit)]); if (tasks.length === 0) return text('No tasks found to check for submissions.'); - const rows: { task: TaskContent; status: SubmissionStatus }[] = []; + const rows: { task: ResolvedTask; status: SubmissionStatus }[] = []; const unavailable: string[] = []; await forEachLimited(tasks, 5, async (task) => { @@ -125,7 +126,7 @@ export function formatGradeState( return 'marked graded, but neither a percentage nor feedback was found'; } -function formatRow({ task, status }: { task: TaskContent; status: SubmissionStatus }): string { +function formatRow({ task, status }: { task: ResolvedTask; status: SubmissionStatus }): string { const state = status.isSubmitted ? 'submitted' : 'not submitted'; // The list does not fetch pages, so it cannot know whether feedback exists; // it says only what the API told it. @@ -144,8 +145,8 @@ async function collectTasks( scope: 'open' | 'finished' | 'all', courseId: string | undefined, limit: number, -): Promise { - const wanted: TaskContent[] = []; +): Promise<(ResolvedTask & { id: string })[]> { + const wanted: ResolvedTask[] = []; if (scope === 'open' || scope === 'all') { wanted.push(...(await context.client.listTasks({ limit }).catch(() => ({ data: [] }))).data); } @@ -160,10 +161,24 @@ async function collectTasks( for (const element of board?.elements ?? []) { if (element.type === 'task') wanted.push({ ...element.content, courseId }); } + // Tasks attached to a topic are not task elements on the course page, so + // they have to be asked for per topic. They are the ones most likely to + // carry a grade: a task old enough to have been marked is usually old + // enough to have dropped out of both task lists. + for (const element of board?.elements ?? []) { + if (element.type !== 'lesson' || !element.content.numberOfPublishedTasks) continue; + const [tasks, links] = await Promise.all([ + context.client.getLessonTasks(element.content.id).catch(() => []), + fetchLessonTaskLinks(context.config, courseId, element.content.id), + ]); + // Only the ones whose id could be recovered: a submission lookup needs it. + for (const task of withScrapedIds(tasks, links)) if (task.id) wanted.push({ ...task, courseId }); + } } const seen = new Set(); return wanted + .filter((task): task is ResolvedTask & { id: string } => Boolean(task.id)) .filter((task) => (courseId ? task.courseId === courseId : true)) .filter((task) => (seen.has(task.id) ? false : (seen.add(task.id), true))) .slice(0, limit); diff --git a/src/store/store.ts b/src/store/store.ts index e50cdcd..3f67a81 100644 --- a/src/store/store.ts +++ b/src/store/store.ts @@ -544,9 +544,12 @@ function fileNode(file: CrawledFile): StoredNode { securityCheckStatus: file.record.securityCheckStatus, at: file.at, }, - // File records are immutable, so identity alone decides change; the size - // is included only to catch an upstream record being rewritten in place. - digest: digestOf([file.record.id, file.record.size]), + // File records are less immutable than they look: `PATCH /file/rename/{id}` + // changes the name in place, keeping the id and the size, and teachers do + // rename files. Leaving the name out made that invisible to what_changed — + // the file simply reappeared under its new name with nothing reported. + // Size still catches a record rewritten in place under the same name. + digest: digestOf([file.record.id, file.record.name, file.record.size]), }; } diff --git a/test/etherpad.test.ts b/test/etherpad.test.ts new file mode 100644 index 0000000..75db231 --- /dev/null +++ b/test/etherpad.test.ts @@ -0,0 +1,39 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { padIdFromUrl } from '../src/core/etherpad.ts'; + +const BASE = 'https://schulcloud.example.org'; + +describe('padIdFromUrl', () => { + it('takes the pad id out of the url the server hands back', () => { + assert.equal(padIdFromUrl(`${BASE}/etherpad/p/g.abc123$65f0e1d2c3b4a5968778695a`, BASE), 'g.abc123$65f0e1d2c3b4a5968778695a'); + }); + + it('keeps the id exactly as encoded', () => { + // Group pad ids contain `$`. Decoding or re-encoding the segment produces + // an id Etherpad does not recognise. + assert.equal(padIdFromUrl(`${BASE}/etherpad/p/g.x%24y`, BASE), 'g.x%24y'); + }); + + it('refuses a url pointing at another host', () => { + // The caller sends an Etherpad session cookie to whatever this returns, and + // the url comes from server configuration — so a mismatch must not be + // followed rather than trusted. + assert.equal(padIdFromUrl('https://evil.test/etherpad/p/g.abc$123', BASE), undefined); + }); + + it('accepts a differing port only when it matches', () => { + assert.equal(padIdFromUrl('http://localhost:4400/etherpad/p/pad1', 'http://localhost:4400'), 'pad1'); + assert.equal(padIdFromUrl('http://localhost:9001/etherpad/p/pad1', 'http://localhost:4400'), undefined); + }); + + it('gives up on a url that is not a pad url', () => { + assert.equal(padIdFromUrl(`${BASE}/dashboard`, BASE), undefined); + assert.equal(padIdFromUrl(`${BASE}/etherpad/p/`, BASE), undefined); + }); + + it('gives up rather than throwing on something that is not a url', () => { + assert.equal(padIdFromUrl('not a url', BASE), undefined); + assert.equal(padIdFromUrl('', BASE), undefined); + }); +}); diff --git a/test/lesson-page.test.ts b/test/lesson-page.test.ts new file mode 100644 index 0000000..62fede1 --- /dev/null +++ b/test/lesson-page.test.ts @@ -0,0 +1,82 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { parseLessonTaskLinks, withScrapedIds } from '../src/core/lesson-page.ts'; + +/** + * Fixtures mirror the legacy topic page as actually served — the task cards it + * renders under `
`, which is the only place a topic-attached + * task's id is exposed at all. + */ +const taskCard = (id: string, name: string) => + `
  • ` + + `
    Fällig: 30.07.2026 15:23
  • `; + +const page = (cards: string) => + `

    Aufgaben (2)

    ` + + `
      ${cards}
    ` + + ``; + +const id = (c: string) => c.repeat(24); + +describe('parseLessonTaskLinks', () => { + it('recovers the ids the API withholds, with their names', () => { + const html = page(taskCard(id('a'), 'Bestandteile einer Kamera') + taskCard(id('b'), 'Eigenschaften einer Sammellinse')); + assert.deepEqual(parseLessonTaskLinks(html), [ + { id: id('a'), name: 'Bestandteile einer Kamera' }, + { id: id('b'), name: 'Eigenschaften einer Sammellinse' }, + ]); + }); + + it('decodes entities in task names', () => { + const html = page(taskCard(id('c'), 'Größe & Form')); + assert.equal(parseLessonTaskLinks(html)[0]?.name, 'Größe & Form'); + }); + + it('reports each task once even when the page links it twice', () => { + // The card title and its action button both point at the same task. + const html = page(taskCard(id('d'), 'Würfelspiel') + taskCard(id('d'), 'Würfelspiel')); + assert.equal(parseLessonTaskLinks(html).length, 1); + }); + + it('returns nothing rather than guessing when the markup is not a topic page', () => { + assert.deepEqual(parseLessonTaskLinks('Anmelden'), []); + }); + + it('ignores homework links that carry no task name', () => { + // A bare link with no aria-label cannot be paired with an API task, so it + // is no use; taking it would produce a task named after nothing. + assert.deepEqual(parseLessonTaskLinks(page(`Aufgabe`)), []); + }); +}); + +describe('withScrapedIds', () => { + it('gives the API tasks the ids from the page, matching on name', () => { + const tasks = [{ name: 'Würfelspiel' }, { name: 'Sammellinse' }]; + const links = [ + { id: id('a'), name: 'Sammellinse' }, + { id: id('b'), name: 'Würfelspiel' }, + ]; + assert.deepEqual( + withScrapedIds(tasks, links).map((t) => t.id), + [id('b'), id('a')], + ); + }); + + it('leaves a task the page does not account for without an id', () => { + // Still worth reporting — the user can see it — but the id-taking tools + // cannot open it, so it must not be handed a fabricated id. + const merged = withScrapedIds([{ name: 'Nur in der API' }], [{ id: id('a'), name: 'Etwas anderes' }]); + assert.equal(merged[0]?.id, undefined); + assert.equal(merged[0]?.name, 'Nur in der API'); + }); + + it('does not overwrite an id the task already has', () => { + const merged = withScrapedIds([{ id: id('f'), name: 'Würfelspiel' }], [{ id: id('a'), name: 'Würfelspiel' }]); + assert.equal(merged[0]?.id, id('f')); + }); + + it('tolerates surrounding whitespace in the API name', () => { + const merged = withScrapedIds([{ name: ' Würfelspiel ' }], [{ id: id('a'), name: 'Würfelspiel' }]); + assert.equal(merged[0]?.id, id('a')); + }); +}); diff --git a/test/store.test.ts b/test/store.test.ts index 1348609..b1e27ca 100644 --- a/test/store.test.ts +++ b/test/store.test.ts @@ -106,6 +106,22 @@ describe('Store', { skip: DB_URL ? false : 'set TEST_DATABASE_URL to run' }, () assert.ok(diff.changed.some((n) => n.nodeId === 'c1-b'), 'board body change detected via digest'); }); + it('reports a renamed file, which keeps its id and size', async () => { + // `PATCH /file/rename/{id}` renames a record in place. The digest once + // covered only id and size on the assumption that file records never + // change, so a rename went unreported — the file just quietly appeared + // under a new name. + const before = await store.latestCrawlId(); + const after = await store.saveSnapshot( + snapshot([{ id: 'c1', title: 'Mathe', boardText: 'Prozentrechnung', files: [{ id: 'f2', name: 'b-v2.pdf', size: 20 }] }]), + 'full', + ); + const diff = await store.diff(before!, after); + assert.ok(diff.changed.some((n) => n.nodeId === 'f2'), 'rename detected'); + assert.ok(!diff.added.some((n) => n.nodeId === 'f2'), 'a rename is not a new file'); + assert.ok(!diff.removed.some((n) => n.nodeId === 'f2'), 'and not a deleted one'); + }); + it('carries other courses forward on a per-course crawl', async () => { await store.saveSnapshot( snapshot([