Skip to content

Export linked grader hint feedback#780

Merged
jon-bell merged 12 commits into
stagingfrom
cursor-export-grader-extra-data-d4e6
May 30, 2026
Merged

Export linked grader hint feedback#780
jon-bell merged 12 commits into
stagingfrom
cursor-export-grader-extra-data-d4e6

Conversation

@jon-bell

@jon-bell jon-bell commented May 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Export hint feedback by the emitted grader_result_tests ids so hints.json only contains rows linked to exported tests.
  • Preserve the existing extra_data field on emitted grader test records.
  • Export per-submission student engagement for error-pinned discussion posts into error-pin-engagement.json.

Testing

  • npm run format
  • npm run lint
  • npm test -- --runTestsByPath tests/unit/assessment-export-tokenization.test.ts tests/unit/assessment-export-selectors.test.ts
  • node -e "const fs=require('fs'); const edge=fs.readFileSync('supabase/functions/cli/commands/assessment.ts','utf8'); const cli=fs.readFileSync('cli/commands/assessment/export.ts','utf8'); if(!edge.includes('kind: \\\"error_pin_engagement\\\"')) throw new Error('edge stream does not emit error_pin_engagement'); if(!edge.includes('.from(\\\"discussion_thread_read_status\\\")')) throw new Error('read status is not exported'); if(!edge.includes('.from(\\\"discussion_thread_likes\\\")')) throw new Error('likes are not exported'); if(!cli.includes('error-pin-engagement.json')) throw new Error('CLI does not write error-pin-engagement.json'); console.log('assessment export includes error-pin engagement read/like wiring');"

Notes

  • Direct npx -y deno check cli/commands/assessment.ts was attempted; it reaches the function but fails on the generated supabase/functions/_shared/SupabaseTypes.d.ts export const Constants ambient declaration before checking this patch.
  • npx tsc --noEmit --pretty false was attempted; it fails on the pre-existing tests/e2e/active-submission-gradebook-db.spec.ts(49,5) nullable string assignment.
Open in Web Open in Cursor 

Summary by CodeRabbit

  • New Features

    • New CLI: submissions export (streams catalog, meta, files; JSON/CSV; include/exclude file filters; optional binary file contents).
    • Export manifests now include grand totals, flags (gradebook_skipped, all_submissions), and error-pin engagement outputs.
  • Improvements

    • Added --skip-gradebook and --all-submissions options and clearer deanonymize/identity (--salt/--i-understand-pii) guidance.
    • Expanded transient error detection and more robust streaming/retry behavior.
    • Instructor build-output sanitization and hint/export enhancements.

Review Change Stack

Co-authored-by: Jonathan Bell <jon@jonbell.net>
@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@jon-bell, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 14 minutes and 4 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: bb5127d0-b60f-4eaa-b719-be9285342bed

📥 Commits

Reviewing files that changed from the base of the PR and between 97b0aa1 and 35cbc9a.

📒 Files selected for processing (12)
  • cli/commands/assessment/export.ts
  • cli/commands/submissions/export.ts
  • cli/utils/concurrency.ts
  • supabase/functions/cli/commands/submissions.ts
  • supabase/functions/cli/utils/filePathMatchers.ts
  • supabase/functions/cli/utils/submissionExportStream.ts
  • supabase/functions/cli/utils/submissionFileExportRecord.ts
  • supabase/functions/cli/utils/submissionFilesExportStream.ts
  • supabase/functions/cli/utils/tokenization.ts
  • tests/unit/sanitize-grading-paths.test.ts
  • tests/unit/submissions-export-file-filters.test.ts
  • tests/unit/submissions-export-files.test.ts

Walkthrough

Adds server-side pepper-backed tokenization (salt+pepper), new CLI export options and validation, streaming export utilities and helpers, rewrites assignment/submissions export to sectioned/paginated streams, emits hints/LLM-derived hints with deduplication, exports error-pin engagement rows, and extends transient-stream error detection and tests.

Changes

Assessment Export with Server-Side Pepper Tokenization

Layer / File(s) Summary
Tokenization foundation with server pepper
supabase/functions/cli/utils/tokenization.ts, supabase/migrations/20260522143000_assessment_export_pepper.sql, supabase/functions/cli/utils/assessmentExportPepper.ts, tests/unit/assessment-export-tokenization.test.ts
Two-step HMAC tokenizer: client salt + deployment pepper (validated via vault RPC/ENV). Adds migration and RPC helper to load/validate pepper; tests updated to require matching salt+pepper for token equality.
CLI identity options & transient error handling
cli/commands/assessment/deanonymize.ts, cli/commands/assessment/export.ts, cli/utils/transientRetry.ts, tests/unit/cli-transient-retry.test.ts
Centralizes identity validation, documents salt/pepper semantics for hash/opaque modes, adds --skip-gradebook/--all-submissions flags and validation, and extends transient error patterns to detect truncated NDJSON streams and resource-limit messages.
CLI export shared utilities
cli/utils/exportFiles.ts
New shared helpers for export commands: identity-mode wiring, secure writers, incremental writeJsonArray, writeCsv, generateRandomSalt, assertExpectedCount, filename sanitization, and ref-token extraction.
Submissions export CLI command
cli/commands/submissions/export.ts, cli/commands/submissions/index.ts
New submissions export yargs builder and handler that streams catalog/meta/files, supports identity modes/salts, include/exclude globs, optional binary inclusion, JSON/CSV formats, concurrency, per-assignment manifests, and a root manifest.
Submission & file stream helpers
supabase/functions/cli/utils/submissionExportStream.ts, supabase/functions/cli/utils/submissionFilesExportStream.ts, supabase/functions/cli/utils/filePathMatchers.ts
Adds paginated streamSubmissions and streamSubmissionFiles, buildFileExportRecord for tests, and glob-to-regex file path matching with include/exclude behavior.
Instructor build-output sanitization
supabase/functions/cli/utils/sanitizeGradingPaths.ts, tests/unit/sanitize-grading-paths.test.ts
Adds path sanitization to redact CI/workflow prefixes, sentinel-based slicing for instructor build output, and tests validating sanitization + sentinel behavior.
Server: assessment preamble & tokenizer integration
supabase/functions/cli/commands/assessment.ts (preamble & setup)
Preamble accepts skip_gradebook, centralizes identity validation, routes non-raw modes through createExportTokenizer(supabase, salt), and replaces large .in() lookups with paged UUID batching for raw-mode user-info resolution.
Server: sectioned assignment export & dispatcher
supabase/functions/cli/commands/assessment.ts (assignment export)
Refactors per-assignment export into sectioned slices (meta/submissions/scores/tests/build_output/engagement/all) with per-section pagination and transient retries, slimmed grader-test extra_data, and instructor build-output preprocessing.
Server: scores, hints, and error-pin engagement
supabase/functions/cli/commands/assessment.ts
Scores now preload review scope; grader tests return { testCount, graderTestIds }; streamHints emits both feedback and LLM-derived hints with deduplication and a source field; error-pin engagement export now emits read_at and liked and aggregates counts end-to-end.
Gradebook & roster export
supabase/functions/cli/commands/assessment.ts
Gradebook and roster export construct hash-mode tokenizers via createExportTokenizer; roster docs clarify opaque tokens are per-run and not reproducible offline.
Shared pawtograder YAML helpers
supabase/functions/_shared/pawtograderYmlHelpers.ts
Adds type-guards and helpers to compute mutation-unit points, graded unit points, and total autograder points across graded parts.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

  • pawtograder/platform#746: Prior work on assessment export implementation that this change extends (tokenization and export streaming surfaces).

Suggested labels

backend, database

Poem

Salt, pepper, streams in line,
Tokens knit where vaults confine,
Sections sing and hints dedupe,
Files and manifests write the loop,
Exported truth in ordered time.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Export linked grader hint feedback' is directly aligned with the primary objective of the PR: exporting hint feedback keyed by grader_result_tests ids into hints.json, and accurately reflects a core change in the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Co-authored-by: Jonathan Bell <jon@jonbell.net>
@argos-ci

argos-ci Bot commented May 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ⚠️ Changes detected (Review) 13 changed, 1 failure May 30, 2026, 5:42 PM

@jon-bell jon-bell marked this pull request as ready for review May 22, 2026 20:02
@jon-bell

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
supabase/functions/cli/commands/assessment.ts (1)

1373-1384: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix test_output_max_bytes truncation to use UTF-8 byte length (not string.length) in assessment.ts (supabase/functions/cli/commands/assessment.ts:1373-1384)

row.output is a string | null, but the code uses output.length and output.slice(0, outputMaxBytes), which count UTF-16 code units—not UTF-8 bytes. This makes both:

  • record.output_truncated decision incorrect for non-ASCII output
  • record.output_full_bytes = output.length incorrect

Truncate by UTF-8 byte length (e.g., iterate with TextEncoder/TextDecoder or equivalent) and set output_full_bytes to the UTF-8 byte length.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/functions/cli/commands/assessment.ts` around lines 1373 - 1384, The
truncation logic in assessment.ts currently uses string.length and slice which
count UTF-16 code units; change it to compute the UTF-8 byte length of
row.output (use TextEncoder to get a Uint8Array), set record.output_full_bytes
to that byte length, and when output exceeds outputMaxBytes truncate by UTF-8
bytes (slice the encoded bytes to outputMaxBytes and decode back to a string
with TextDecoder) so record.output contains a properly truncated UTF-8-safe
string and record.output_truncated is set accordingly; update the branches that
reference output, outputMaxBytes, record.output, record.output_truncated, and
record.output_full_bytes to use this byte-based approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cli/commands/assessment/export.ts`:
- Around line 520-530: The pagination loop using batchIndex and the returned
next cursor (from streamAssignmentSection ->
page.endRecord.next_score_review_batch_index) must guard against non-advancing
cursors; update the loop to break (or throw) when next is not a number OR when
next <= batchIndex to prevent infinite loops and repeated edge calls. Do the
same change for the sibling loops that read next_answer_review_batch_index /
other next_*_batch_index cursors (the loops that call mergeBuckets,
addPageCounts, assertExpectedCount), i.e. only assign batchIndex = next when
typeof next === "number" && next > batchIndex, otherwise exit the loop
(optionally log a warning).

In `@supabase/functions/cli/commands/assessment.ts`:
- Around line 758-773: The switch over section (derived from params.section in
assessment.ts) currently treats unknown values as "all" via the default case,
which causes typos like "score" to run exportAssignmentAll; change this to
explicitly reject invalid section values by validating params.section (or the
computed const section) against the allowed AssignmentExportSection values and
replacing the switch default with logic that throws/returns a 400 Bad Request
(or calls the existing request-error helper) when the value is not one of
"meta","submissions","scores","tests","engagement","all"; update the switch in
the same function to only handle the known cases (exportAssignmentMeta,
exportAssignmentSubmissions, exportAssignmentScores, exportAssignmentTests,
exportAssignmentEngagement, exportAssignmentAll) and ensure unknown inputs
produce an immediate error response instead of falling back to
exportAssignmentAll.
- Around line 454-468: slimExtraDataForExport currently destructures prompt and
result into _prompt/_result but leaves them unused which trips
`@typescript-eslint/no-unused-vars`; after the destructuring in
slimExtraDataForExport (where llmObj and { prompt: _prompt, result: _result,
...llmRest } are declared) explicitly mark those vars as used by adding short
no-op references (for example: void _prompt; void _result;) before the return so
ESLint stops complaining while preserving the existing shape logic for llmObj
and llmRest.

In `@tests/unit/assessment-export-tokenization.test.ts`:
- Line 13: Update the import statement in the test to use the project-root alias
instead of the relative path: replace the relative import that currently pulls
createTokenizer, generateRandomSalt, and base32 from
"../../supabase/functions/cli/utils/tokenization" with the alias import from
"`@/supabase/functions/cli/utils/tokenization`" so the test imports
createTokenizer, generateRandomSalt, and base32 via the "`@/`..." alias.

---

Outside diff comments:
In `@supabase/functions/cli/commands/assessment.ts`:
- Around line 1373-1384: The truncation logic in assessment.ts currently uses
string.length and slice which count UTF-16 code units; change it to compute the
UTF-8 byte length of row.output (use TextEncoder to get a Uint8Array), set
record.output_full_bytes to that byte length, and when output exceeds
outputMaxBytes truncate by UTF-8 bytes (slice the encoded bytes to
outputMaxBytes and decode back to a string with TextDecoder) so record.output
contains a properly truncated UTF-8-safe string and record.output_truncated is
set accordingly; update the branches that reference output, outputMaxBytes,
record.output, record.output_truncated, and record.output_full_bytes to use this
byte-based approach.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 87d5ec32-d86b-420a-987e-d84f6df2646f

📥 Commits

Reviewing files that changed from the base of the PR and between 8bcd86b and d1a74b9.

📒 Files selected for processing (8)
  • cli/commands/assessment/deanonymize.ts
  • cli/commands/assessment/export.ts
  • cli/utils/transientRetry.ts
  • supabase/functions/cli/commands/assessment.ts
  • supabase/functions/cli/utils/tokenization.ts
  • supabase/migrations/20260522143000_assessment_export_pepper.sql
  • tests/unit/assessment-export-tokenization.test.ts
  • tests/unit/cli-transient-retry.test.ts

Comment thread cli/commands/assessment/export.ts
Comment thread supabase/functions/cli/commands/assessment.ts
Comment thread supabase/functions/cli/commands/assessment.ts Outdated
Comment thread tests/unit/assessment-export-tokenization.test.ts
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (7)
cli/utils/exportFiles.ts (1)

54-80: ⚖️ Poor tradeoff

Shared utilities are duplicated in cli/commands/assessment/export.ts.

The writeJsonArray, sanitizeForFilename, timestamp, generateRandomSalt, and base32Encode functions in this shared module are also defined locally in cli/commands/assessment/export.ts. Consider updating that file to import from this shared module to maintain a single source of truth.

Also applies to: 94-127

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/utils/exportFiles.ts` around lines 54 - 80, The functions writeJsonArray,
sanitizeForFilename, timestamp, generateRandomSalt, and base32Encode are
duplicated in cli/commands/assessment/export.ts; remove the local copies there
and import these utilities from cli/utils/exportFiles (export them from this
module if not already exported). Update the assessment export code to reference
the shared symbols (writeJsonArray, sanitizeForFilename, timestamp,
generateRandomSalt, base32Encode) via imports, ensure any TypeScript
types/exports are adjusted in cli/utils/exportFiles to be public, and run a
quick compile to fix any leftover import/type errors.
supabase/functions/cli/commands/submissions.ts (1)

606-725: 💤 Low value

Consider using a Set for section validation like assessment.ts does.

The assessment export command uses ASSIGNMENT_EXPORT_SECTIONS set with explicit validation before the switch. This submissions export relies on the final throw at line 724 after the if-else chain. While functionally correct, the explicit set validation pattern is clearer and easier to maintain.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/functions/cli/commands/submissions.ts` around lines 606 - 725, The
handler handleSubmissionsExport currently falls through to a final throw for
invalid section values; add an explicit Set of allowed sections (like
ASSIGNMENT_EXPORT_SECTIONS used in assessment.ts) and validate params.section
(or computed section variable) at the top of the function—if the section is not
in the Set throw a CLICommandError immediately—so the allowed values are
declared in one place and the rest of the if/branch logic can assume a valid
section.
cli/commands/submissions/export.ts (1)

351-363: 💤 Low value

runWithConcurrency is duplicated from cli/commands/assessment/export.ts.

Consider extracting this utility to a shared module (e.g., cli/utils/concurrency.ts) to avoid code duplication and ensure consistent behavior across export commands.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/commands/submissions/export.ts` around lines 351 - 363, The
runWithConcurrency function is duplicated; extract it into a shared utility
module (e.g., utils/concurrency.ts) and replace the duplicate implementations by
importing that exported function. Specifically, move the implementation of
runWithConcurrency (the async function that takes tasks and concurrency, manages
nextIdx, workers array and fills results) into the new module, export it, update
this file to import runWithConcurrency instead of declaring it, and update the
other file (assessment export) to import the same exported function so both use
the single shared implementation.
tests/unit/submissions-export-files.test.ts (1)

5-6: ⚡ Quick win

Use @/* aliases for these test imports.

These should use root aliases instead of relative traversal.

Proposed change
-import { createTokenizer } from "../../supabase/functions/cli/utils/tokenization";
-import { buildFileExportRecord } from "../../supabase/functions/cli/utils/submissionFilesExportStream";
+import { createTokenizer } from "`@/supabase/functions/cli/utils/tokenization`";
+import { buildFileExportRecord } from "`@/supabase/functions/cli/utils/submissionFilesExportStream`";

As per coding guidelines, **/*.{ts,tsx,js,jsx}: Use @/* path alias to reference files from project root.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/submissions-export-files.test.ts` around lines 5 - 6, Replace the
relative traversal imports in the test with root path aliases: change the import
of createTokenizer from "../../supabase/functions/cli/utils/tokenization" to
"`@/supabase/functions/cli/utils/tokenization`" and change the import of
buildFileExportRecord from
"../../supabase/functions/cli/utils/submissionFilesExportStream" to
"`@/supabase/functions/cli/utils/submissionFilesExportStream`" so the test imports
use the `@/` alias and reference the same exported symbols createTokenizer and
buildFileExportRecord.
supabase/functions/cli/utils/filePathMatchers.ts (1)

36-44: ⚡ Quick win

Cache compiled glob regexes in matcher hot path.

Line 37 recompiles the same pattern repeatedly for each file row. A tiny cache avoids repeated compile overhead during large exports.

Proposed refactor
+const globRegExpCache = new Map<string, RegExp>();
+
 function matchesGlob(pattern: string, path: string): boolean {
-  const re = globPatternToRegExp(pattern);
+  let re = globRegExpCache.get(pattern);
+  if (!re) {
+    re = globPatternToRegExp(pattern);
+    globRegExpCache.set(pattern, re);
+  }
   if (re.test(path)) return true;
   // Patterns without / also match the basename (e.g. *.java matches src/Main.java).

Also applies to: 54-58

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/functions/cli/utils/filePathMatchers.ts` around lines 36 - 44, The
matchesGlob function recompiles the same glob pattern for every path; add a
module-level cache Map<string, RegExp> (e.g., compiledGlobCache) and use it
inside matchesGlob: look up pattern, call globPatternToRegExp only if not
cached, store the RegExp, then reuse it for both the full-path test and the
basename test; apply the same caching change to the similar matcher function
around lines 54-58 that also calls globPatternToRegExp so both hot paths reuse
compiled regexes.
tests/unit/submissions-export-file-filters.test.ts (1)

5-5: ⚡ Quick win

Use the @/* alias for this test import.

Replace the relative path with the project-root alias.

Proposed change
-import { matchesSubmissionFilePath } from "../../supabase/functions/cli/utils/filePathMatchers";
+import { matchesSubmissionFilePath } from "`@/supabase/functions/cli/utils/filePathMatchers`";

As per coding guidelines, **/*.{ts,tsx,js,jsx}: Use @/* path alias to reference files from project root.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/submissions-export-file-filters.test.ts` at line 5, The import in
tests/unit/submissions-export-file-filters.test.ts should use the project-root
alias instead of a relative path; update the import that references
matchesSubmissionFilePath to use the "`@/`..." alias (e.g., import from
"`@/supabase/functions/cli/utils/filePathMatchers`") so the test follows the `@/*`
path alias convention and resolves consistently across the project.
tests/unit/sanitize-grading-paths.test.ts (1)

5-9: ⚡ Quick win

Switch test import to the @/* root alias.

Use the project-root alias here instead of a deep relative path.

Proposed change
 import {
   prepareInstructorBuildOutput,
   sanitizeGradingPaths,
   sliceOutputFromSentinel
-} from "../../supabase/functions/cli/utils/sanitizeGradingPaths";
+} from "`@/supabase/functions/cli/utils/sanitizeGradingPaths`";

As per coding guidelines, **/*.{ts,tsx,js,jsx}: Use @/* path alias to reference files from project root.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/sanitize-grading-paths.test.ts` around lines 5 - 9, Replace the
deep relative import in the test with the project-root alias: update the import
that currently brings in prepareInstructorBuildOutput, sanitizeGradingPaths, and
sliceOutputFromSentinel from
"../../supabase/functions/cli/utils/sanitizeGradingPaths" to use the "`@/`…" root
alias (e.g. import { prepareInstructorBuildOutput, sanitizeGradingPaths,
sliceOutputFromSentinel } from
"`@/supabase/functions/cli/utils/sanitizeGradingPaths`"); ensure the symbols
remain the same and the path resolves via the project's path alias
configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cli/commands/submissions/export.ts`:
- Around line 281-284: The loop that paginates file batches only checks typeof
next_files_batch_index and can infinite-loop if the server returns the same or
lower index; update the loop that uses filesBatchIndex and
next_files_batch_index to detect non-advancing cursors (e.g., require next >
filesBatchIndex) and break or throw when the cursor does not advance; preferably
reuse the existing advanceBatchCursor helper (or implement equivalent logic) to
compare next and filesBatchIndex, log an error message referencing
next_files_batch_index and filesBatchIndex, and stop pagination to avoid
infinite loops.

In `@supabase/functions/cli/utils/submissionExportStream.ts`:
- Around line 21-30: In streamSubmissions, add an early guard that validates the
mode/tokenizer invariant: if mode is 'hash' or 'opaque' and tokenizer is null,
throw a clear error (e.g., throw new Error(...)) before any writing occurs so we
don't emit raw IDs for submission/subject/group; place this check near the start
of streamSubmissions (after includeOrdinal is set) to fail fast and document the
expectation in the error message.

In `@supabase/functions/cli/utils/submissionFilesExportStream.ts`:
- Around line 79-86: The warning records are currently emitting raw
row.submission_id to writer.write (kind: "warning") which leaks real IDs when
tokenized mode is enabled; update the two places that write these warnings (the
block using row.submission_id at lines shown and the similar block at 94-99) to
check the module's tokenized/identity mode flag and, if tokenized mode is
active, replace submission_id with the tokenized field (e.g., submission_token
or tokenized_submission_id) or omit the raw submission_id, using the existing
tokenization helper in this module (or add one) to generate the token; ensure
you only change the payload for writer.write calls that include submission_id so
identity-mode consistency is preserved.
- Around line 157-160: The truthy check for input.content_base64 in
buildFileExportRecord treats an empty string (valid base64 for zero bytes) as
omitted; change the condition so it checks for explicit null/undefined (e.g.,
input.content_base64 !== undefined && input.content_base64 !== null) instead of
a truthy check, then assign record.content_base64 = input.content_base64 and set
record.binary_omitted = false when input.is_binary && input.withBinary and the
content is present (including ""), otherwise mark binary_omitted true; reference
buildFileExportRecord, input.is_binary, input.withBinary, input.content_base64,
record.content_base64, and record.binary_omitted when making the change.

---

Nitpick comments:
In `@cli/commands/submissions/export.ts`:
- Around line 351-363: The runWithConcurrency function is duplicated; extract it
into a shared utility module (e.g., utils/concurrency.ts) and replace the
duplicate implementations by importing that exported function. Specifically,
move the implementation of runWithConcurrency (the async function that takes
tasks and concurrency, manages nextIdx, workers array and fills results) into
the new module, export it, update this file to import runWithConcurrency instead
of declaring it, and update the other file (assessment export) to import the
same exported function so both use the single shared implementation.

In `@cli/utils/exportFiles.ts`:
- Around line 54-80: The functions writeJsonArray, sanitizeForFilename,
timestamp, generateRandomSalt, and base32Encode are duplicated in
cli/commands/assessment/export.ts; remove the local copies there and import
these utilities from cli/utils/exportFiles (export them from this module if not
already exported). Update the assessment export code to reference the shared
symbols (writeJsonArray, sanitizeForFilename, timestamp, generateRandomSalt,
base32Encode) via imports, ensure any TypeScript types/exports are adjusted in
cli/utils/exportFiles to be public, and run a quick compile to fix any leftover
import/type errors.

In `@supabase/functions/cli/commands/submissions.ts`:
- Around line 606-725: The handler handleSubmissionsExport currently falls
through to a final throw for invalid section values; add an explicit Set of
allowed sections (like ASSIGNMENT_EXPORT_SECTIONS used in assessment.ts) and
validate params.section (or computed section variable) at the top of the
function—if the section is not in the Set throw a CLICommandError immediately—so
the allowed values are declared in one place and the rest of the if/branch logic
can assume a valid section.

In `@supabase/functions/cli/utils/filePathMatchers.ts`:
- Around line 36-44: The matchesGlob function recompiles the same glob pattern
for every path; add a module-level cache Map<string, RegExp> (e.g.,
compiledGlobCache) and use it inside matchesGlob: look up pattern, call
globPatternToRegExp only if not cached, store the RegExp, then reuse it for both
the full-path test and the basename test; apply the same caching change to the
similar matcher function around lines 54-58 that also calls globPatternToRegExp
so both hot paths reuse compiled regexes.

In `@tests/unit/sanitize-grading-paths.test.ts`:
- Around line 5-9: Replace the deep relative import in the test with the
project-root alias: update the import that currently brings in
prepareInstructorBuildOutput, sanitizeGradingPaths, and sliceOutputFromSentinel
from "../../supabase/functions/cli/utils/sanitizeGradingPaths" to use the "`@/`…"
root alias (e.g. import { prepareInstructorBuildOutput, sanitizeGradingPaths,
sliceOutputFromSentinel } from
"`@/supabase/functions/cli/utils/sanitizeGradingPaths`"); ensure the symbols
remain the same and the path resolves via the project's path alias
configuration.

In `@tests/unit/submissions-export-file-filters.test.ts`:
- Line 5: The import in tests/unit/submissions-export-file-filters.test.ts
should use the project-root alias instead of a relative path; update the import
that references matchesSubmissionFilePath to use the "`@/`..." alias (e.g., import
from "`@/supabase/functions/cli/utils/filePathMatchers`") so the test follows the
`@/*` path alias convention and resolves consistently across the project.

In `@tests/unit/submissions-export-files.test.ts`:
- Around line 5-6: Replace the relative traversal imports in the test with root
path aliases: change the import of createTokenizer from
"../../supabase/functions/cli/utils/tokenization" to
"`@/supabase/functions/cli/utils/tokenization`" and change the import of
buildFileExportRecord from
"../../supabase/functions/cli/utils/submissionFilesExportStream" to
"`@/supabase/functions/cli/utils/submissionFilesExportStream`" so the test imports
use the `@/` alias and reference the same exported symbols createTokenizer and
buildFileExportRecord.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4da1b1ac-8872-4784-9639-79232e03f1e3

📥 Commits

Reviewing files that changed from the base of the PR and between d1a74b9 and 97b0aa1.

📒 Files selected for processing (17)
  • cli/commands/assessment/export.ts
  • cli/commands/submissions/export.ts
  • cli/commands/submissions/index.ts
  • cli/utils/exportFiles.ts
  • supabase/functions/_shared/pawtograderYmlHelpers.ts
  • supabase/functions/cli/commands/assessment.ts
  • supabase/functions/cli/commands/submissions.ts
  • supabase/functions/cli/utils/assessmentExportPepper.ts
  • supabase/functions/cli/utils/exportIdentity.ts
  • supabase/functions/cli/utils/filePathMatchers.ts
  • supabase/functions/cli/utils/sanitizeGradingPaths.ts
  • supabase/functions/cli/utils/submissionExportStream.ts
  • supabase/functions/cli/utils/submissionFilesExportStream.ts
  • tests/unit/sanitize-grading-paths.test.ts
  • tests/unit/submissions-export-file-filters.test.ts
  • tests/unit/submissions-export-files.test.ts
  • tests/unit/viewAsStudentDataMask.test.ts

Comment thread cli/commands/submissions/export.ts
Comment thread supabase/functions/cli/utils/submissionExportStream.ts
Comment thread supabase/functions/cli/utils/submissionFilesExportStream.ts
Comment thread supabase/functions/cli/utils/submissionFilesExportStream.ts Outdated
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 30, 2026
@jon-bell

Copy link
Copy Markdown
Contributor Author

Addressed the remaining CodeRabbit feedback plus a new privacy hardening:

Resolved review threads (4 major/minor):

  • Guard against a non-advancing next_files_batch_index cursor in submissions/export.ts (throws instead of looping forever).
  • streamSubmissions now enforces the mode/tokenizer invariant up front (hash/opaque must have a tokenizer; raw must not).
  • File warning records emit a tokenized submission ref instead of a raw submission_id.
  • buildFileExportRecord treats a zero-byte binary ("") as present, not omitted.

Nitpicks:

  • submissions.ts now validates section against a SUBMISSIONS_EXPORT_SECTIONS set (mirrors assessment export).
  • filePathMatchers caches compiled glob regexes.
  • Extracted runWithConcurrency to cli/utils/concurrency.ts and deduped assessment/export.ts to import the shared writeJson/writeJsonArray/assertExpectedCount/sanitizeForFilename/timestamp/generateRandomSalt from cli/utils/exportFiles.ts.
  • Switched the three unit-test imports to the @/* alias.

New: obfuscate repository name + commit SHA in non-raw submission exports.
A student repo name embeds their GitHub handle and a commit SHA can be looked up on GitHub, so in hash/opaque mode streamSubmissions now emits stable HMAC tokens (new repository and commit token kinds) for repository and sha instead of the raw values. Raw mode is unchanged. The autograder/grader-repo metadata (instructor-owned, not student PII) is intentionally left as-is.

…ion exports

- Guard non-advancing next_files_batch_index cursor (submissions/export.ts)
- Enforce mode/tokenizer invariant in streamSubmissions (fail fast)
- Emit tokenized submission ref (not raw submission_id) in file warning records
- Treat zero-byte binary ("") as present in buildFileExportRecord
- Validate section via SUBMISSIONS_EXPORT_SECTIONS set
- Cache compiled glob regexes in filePathMatchers
- Extract runWithConcurrency to cli/utils/concurrency.ts; dedup assessment export helpers
- Use @/* alias in three unit-test imports
- Obfuscate student repository name and commit SHA (new repository/commit token
  kinds) in hash/opaque submission exports; raw mode unchanged

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

jon-bell and others added 2 commits May 30, 2026 13:13
The submissions-export-files unit test imported buildFileExportRecord from
submissionFilesExportStream.ts, which transitively pulls Deno-only modules
(supabase client, .ts-extension imports) into the Next.js tsc program and
broke the type-check. Move the pure helper into a self-contained
submissionFileExportRecord.ts (matching the repo's testable-CLI-module
convention), re-export it for the Deno consumer, and point the test there.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jon-bell jon-bell merged commit 356a02e into staging May 30, 2026
9 of 11 checks passed
@jon-bell jon-bell deleted the cursor-export-grader-extra-data-d4e6 branch May 30, 2026 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants