Unified per-assignment student repository configuration (#698, #699, #700)#781
Open
jon-bell wants to merge 7 commits into
Open
Unified per-assignment student repository configuration (#698, #699, #700)#781jon-bell wants to merge 7 commits into
jon-bell wants to merge 7 commits into
Conversation
…700) Adds a new "Student Repositories" section on the assignment edit/create form that lets instructors pick one of four modes: staff-only template (current default), student-visible template with student forks, fork from a prior assignment's per-student repos (#700), or no repository at all (#699). The same UI exposes per-assignment GitHub branch-protection rules (#698). * New assignment_repo_mode enum and protect_* columns on assignments; existing rows backfill to template_only_staff with block_force_push=true. * createRepo gains a creation_method: "template" | "fork" path, and branch protection now flows through applyBranchProtectionRuleset which builds rules from a BranchProtectionConfig (idempotent create / update / delete) instead of always installing non_fast_forward. * assignment-create-handout-repo, assignment-create-all-repos, and autograder-create-repos-for-student route per-student repo creation through new pure helpers (handoutRepoStrategy, repoCreationStrategy, branchProtection) that are exercised by Jest unit tests with a mocked GitHub layer, so the dispatch logic is covered without real network. * Submissions can now omit repository/sha for upload-based no-repo submissions; new create_no_repo_submission RPC + createNoRepoSubmission client wrapper handle the upload flow.
Contributor
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 37 minutes and 23 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (28)
WalkthroughAdds per-assignment repository modes and branch-protection config, fork-aware repo creation strategy and GitHub wrapper changes, no-repo submission RPCs and edge wrappers, DB migrations and TS types, UI form fields and guards, async-worker/enqueue wiring, and many unit and e2e tests. ChangesRepository Mode Configuration & Submission Strategy
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
Fifth value on assignment_repo_mode for assignments with no git repository and no student-uploaded artifact (e.g. presentations, oral exams). The grading flow still needs a submission row to attach reviews to, so add an instructor-only create_manual_submission RPC that produces a stub (repository=null, sha=null, submitted_via='manual'); idempotent per profile/group. create_no_repo_submission keeps rejecting non-'none' modes so students can't upload for no_submission assignments. Handout / per-student repo creation paths treat no_submission like 'none' (skip), branch protection is disallowed by the existing constraint extended to cover both modes, and the prior-assignment dropdown filters out both modes since neither has repos to fork. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/functions/_shared/GitHubWrapper.ts (1)
916-970:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply branch protection in the “repo already exists” fallback path.
When Line 916 handles an existing repo, the function returns at Line 970 without syncing
branch_protection, so rules can drift from assignment config.Suggested fix
try { await retryWithBackoff( () => octokit.request("PUT /repos/{owner}/{repo}/actions/permissions", { owner: org, repo: repoName, enabled: true, allowed_actions: "all" }), 3, 1000, scope ); } catch (actionsErr) { console.error("Error enabling GitHub Actions for pre-existing repo", actionsErr); scope?.setTag("enable_actions_failed", "true"); Sentry.captureException(actionsErr, scope); } + try { + await applyBranchProtectionRuleset(org, repoName, branch_protection, scope); + } catch (rulesetError) { + console.error("Error applying branch protection ruleset for pre-existing repo", rulesetError); + scope?.setTag("ruleset_creation_failed", "true"); + Sentry.captureException(rulesetError, scope); + } return heads.data.object.sha as string;🤖 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/_shared/GitHubWrapper.ts` around lines 916 - 970, The fallback path that handles "Name already exists on this account" returns without applying branch protection, so add the same branch protection sync that the fresh-create path runs: after enabling Actions (and before returning heads.data.object.sha) call the existing branch-protection logic using retryWithBackoff and octokit.request to set the branch protection rules based on the branch_protection config (or invoke the helper used elsewhere in this file if present), and capture/report any errors (set appropriate Sentry tags like patch_existing_repo_settings_failed or similar) to keep branch rules in sync.app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsx (1)
1411-1421:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid an empty staff “Commit History” dialog for upload submissions.
When repository fields are missing, the dialog body renders nothing. Add a clear fallback message (or disable the trigger) for no-repo submissions.
🤖 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 `@app/course/`[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsx around lines 1411 - 1421, The current JSX conditionally renders StaffCommitHistory only when submission.repository_id and submission.repository are present, which leaves upload/no-repo submissions with an empty UI; update the render logic in layout.tsx around the StaffCommitHistory block so that when submission.repository_id === null || submission.repository === null you either (a) render a clear fallback UI element (e.g., a non-interactive message like "No commit history available for upload submissions") or (b) render the same trigger disabled with an explanatory tooltip/message; make the change by modifying the conditional that references submission.repository_id and submission.repository and/or by adding a small Fallback component or prop to StaffCommitHistory to show the message instead of an empty dialog.
🧹 Nitpick comments (5)
tests/unit/no-repo-submission.test.ts (1)
5-5: ⚡ Quick winUse the project-root alias for this import.
Switch the relative path to
@/lib/edgeFunctionsto match repo import conventions.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/no-repo-submission.test.ts` at line 5, The import in tests/unit/no-repo-submission.test.ts uses a relative path; replace the relative import with the project-root alias by importing createNoRepoSubmission from "`@/lib/edgeFunctions`" instead of "../../lib/edgeFunctions" so the test follows the repo's path-alias convention and resolves from the project root (update the import that references createNoRepoSubmission).tests/e2e/active-submission-gradebook-db.spec.ts (1)
49-49: ⚡ Quick winConsider failing fast if slug is unexpectedly null.
The
?? ""fallback satisfies TypeScript but may obscure test failures. SinceinsertAssignmentexplicitly setsassignment_slug(line 46), a null slug indicates a broken test fixture. Passing""will causegetAssignmentGradebookColumnto fail later with a cryptic "missing row" error rather than surfacing the root cause here.🔍 Proposed fail-fast assertion
-assignmentSlug = assignment.slug ?? ""; +if (!assignment.slug) { + throw new Error(`Assignment ${assignmentId} was created without a slug`); +} +assignmentSlug = assignment.slug;🤖 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/e2e/active-submission-gradebook-db.spec.ts` at line 49, Replace the silent fallback assignment for assignmentSlug with a fail-fast assertion: after calling insertAssignment (which sets assignment_slug), assert that assignment.slug is non-null/defined (e.g., using your test assertion helper or throw an Error) and then assign assignmentSlug = assignment.slug; this ensures tests fail immediately when insertAssignment returns a broken fixture instead of propagating a cryptic error into getAssignmentGradebookColumn.supabase/functions/autograder-create-repos-for-student/index.ts (1)
9-14: ⚡ Quick winUse the repo-root alias for these new shared imports.
Please switch the new
../_shared/...imports to@/...so this file follows the repo-wide import rule.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 `@supabase/functions/autograder-create-repos-for-student/index.ts` around lines 9 - 14, Update the import paths to use the repo-root alias instead of relative paths: change the imports that currently reference "../_shared/repoCreationStrategy.ts" and "../_shared/branchProtection.ts" to use "`@/` _shared/..." (e.g., "`@/` _shared/repoCreationStrategy.ts" and "`@/` _shared/branchProtection.ts") while keeping the same exported symbols (resolveRepoCreationStrategy, AssignmentForRepoCreation, SourceRepoRow, BranchProtectionConfig) so the rest of the file continues to work with those identifiers.supabase/functions/assignment-create-all-repos/index.ts (1)
10-17: ⚡ Quick winUse the repo-root alias for these new shared imports.
Please switch the new
../_shared/...imports to@/...so this file follows the repo-wide import rule.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 `@supabase/functions/assignment-create-all-repos/index.ts` around lines 10 - 17, Summary: The new shared imports use relative paths; change them to the repo-root alias. Update the import lines that bring in resolveRepoCreationStrategy, AssignmentForRepoCreation, SourceRepoRow, StudentIdentity from "../_shared/repoCreationStrategy.ts" and BranchProtectionConfig from "../_shared/branchProtection.ts" to use the project alias (replace "../_shared/..." with "`@/` _shared/..." i.e. "`@/` _shared/repoCreationStrategy.ts" and "`@/` _shared/branchProtection.ts") so the file follows the repo-wide "`@/`..." import rule and matches other files' import style.supabase/functions/assignment-create-handout-repo/index.ts (1)
5-13: ⚡ Quick winUse the repo-root alias for these new shared imports.
Please switch the new
../_shared/...imports to@/...so this file follows the repo-wide import rule.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 `@supabase/functions/assignment-create-handout-repo/index.ts` around lines 5 - 13, The file imports shared utilities using relative paths (applyBranchProtectionRuleset, createRepo, syncRepoPermissions, updateAutograderWorkflowHash, assertUserIsInstructorOrServiceRole, UserVisibleError, wrapRequestHandler, Database, resolveHandoutRepoAction, HandoutSourceAssignment) — update those import specifiers from "../_shared/..." to use the repo-root alias "`@/`..." so they follow the project rule (e.g., replace "../_shared/GitHubWrapper.ts" with "`@/` _shared/GitHubWrapper.ts" and likewise for HandlerUtils.ts, SupabaseTypes.d.ts, and handoutRepoStrategy.ts); ensure the import names remain unchanged and that any TypeScript path mapping supports the "`@/`..." alias.
🤖 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 `@app/course/`[course_id]/manage/assignments/new/page.tsx:
- Around line 132-141: The toast messages still display repo-specific
progress/success even when repoMode === "none"; update the logic around repo
creation in the page (where repoMode is checked and assignmentCreateHandoutRepo
/ assignmentCreateSolutionRepo are called) so that any toast or progress text
about creating handout/solution repositories is only shown when repoMode !==
"none", and when repoMode === "none" show a generic "Assignment created" (or
similar) toast tied to the assignment creation using data.id and course_id
instead of repo-specific messages.
In `@lib/edgeFunctions.ts`:
- Around line 146-159: The RPC response from "create_no_repo_submission" is
currently assumed to be a numeric submission ID but may be null or non-numeric;
update the handler that calls supabase.rpc (the create_no_repo_submission call
in this module) to validate the returned data: check that data is a non-null
number (e.g., typeof data === "number" and Number.isFinite(data)); if validation
fails, capture the error with Sentry and throw an EdgeFunctionError with a clear
message (e.g., "Invalid submission id returned from RPC") and recoverable:
false, otherwise return the validated number.
In `@supabase/functions/_shared/GitHubAsyncTypes.ts`:
- Around line 107-119: The worker currently ignores the new fields and always
runs the legacy handout-sync path; update the github-async-worker to read
sync_strategy and upstream_repo_full_name from the incoming payload
(sync_strategy, upstream_repo_full_name) and branch: if sync_strategy ===
"fork_merge_upstream" (or upstream_repo_full_name is present for
fork_from_prior_assignment) invoke the fork-sync flow that calls GitHub's
fork-sync/upstream merge endpoint using upstream_repo_full_name as the upstream
target, otherwise default to the existing "template_pr" handout-sync path;
ensure the default behavior remains when sync_strategy is undefined.
In `@supabase/functions/_shared/GitHubWrapper.ts`:
- Around line 36-41: The import in GitHubWrapper.ts currently uses a relative
path; update it to use the project-root alias (`@/`*) by replacing the relative
import with an alias import that still brings in BRANCH_PROTECTION_RULESET_NAME,
BranchProtectionConfig, DEFAULT_BRANCH_PROTECTION, and
planBranchProtectionAction from the same module, ensuring the exported symbols
remain unchanged and the import statement resolves via the `@/`* path alias.
- Around line 1876-1896: The current block in GitHubWrapper.ts only adds the
`${courseSlug}-students` team via the octokit.request PUT when
options.studentTeamPermission is set, but never removes that team's repo access
if student visibility is later disabled; update the logic around
options.studentTeamPermission and studentsTeamSlug so that when
options.studentTeamPermission is falsy you detect an existing team entry in
teamsWithAccess (matching t.slug === studentsTeamSlug) and remove its repo
access via octokit.request (DELETE
/orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}), set madeChanges = true, and
add an appropriate scope?.addBreadcrumb call to record the removal; keep the
existing grant code path for the non-empty permission case.
In `@supabase/functions/_shared/handoutRepoStrategy.ts`:
- Line 5: Replace the relative import for AssignmentForRepoCreation and
AssignmentRepoMode from "./repoCreationStrategy.ts" with the project-root path
alias (e.g., "`@/`...") so the module is imported via the `@/`* alias; update the
import that references repoCreationStrategy.ts to use the `@/` path (keeping the
exported symbols AssignmentForRepoCreation and AssignmentRepoMode unchanged).
In `@supabase/functions/_shared/repoCreationStrategy.ts`:
- Line 6: Replace the relative import of BranchProtectionConfig with the
project-root alias form: change the import that currently references
"./branchProtection.ts" so it uses the "`@/`..." alias (importing
BranchProtectionConfig) to follow the repository guideline for using `@/`* imports
for shared modules; ensure the symbol name BranchProtectionConfig remains
unchanged and the new path resolves to the same module.
In `@supabase/functions/assignment-create-all-repos/index.ts`:
- Around line 228-240: The Supabase query that populates sourceAssignmentRepos
ignores errors and treats failures as an empty array, causing
fork_from_prior_assignment to misclassify repos; update the adminSupabase query
handling in assignment-create-all-repos/index.ts so you check the returned error
(e.g., the response.error or equivalent) after
adminSupabase.from(...).select(...).eq(...).limit(...), and if an error exists,
surface it (throw or return a failing result) instead of setting
sourceAssignmentRepos = []; ensure resolveRepoCreationStrategy still receives an
explicit failure rather than an empty list so missing_source is not incorrectly
applied.
In `@supabase/functions/assignment-create-handout-repo/index.ts`:
- Around line 115-118: The assignment's template_repo is being persisted
prematurely; move the adminSupabase .from("assignments").update({ template_repo:
`${handoutRepoOrg}/${handoutRepoName}` }).eq("id", assignment_id) call so it
runs only after the GitHub handout repo creation and all subsequent steps
succeed (i.e., after createRepo completes, after the permission sync step
finishes, and after the workflow-hash update completes); ensure any failures in
createRepo, permission sync, or workflow-hash update abort without writing
template_repo to the DB so the assignment cannot point to a non-existent repo.
- Around line 124-131: Coalesce the nullable protection fields before sending to
GitHub: in the object where is_template_repo/isTemplateRepo and
branch_protection are set (the block that maps blockForcePush,
requirePullRequest, requiredReviewers), replace direct uses of
assignment.protect_* with null-coalesced values (e.g.,
assignment.protect_block_force_push ?? true,
assignment.protect_require_pull_request ?? false, and
assignment.protect_required_reviewers ?? [] or similar default) so defaults
(especially blockForcePush=true) are preserved; apply the same change to the
other identical branch_protection block later in the file.
In `@supabase/functions/autograder-create-repos-for-student/index.ts`:
- Around line 35-45: The DB query currently swallows failures by returning (data
?? []) which hides Supabase errors and causes resolveRepoCreationStrategy to
misclassify failures as missing_source; change the
adminSupabase.from(...).select(...).eq(...).limit(2000) call handling to inspect
the returned { data, error } (or similar) and if error is present throw that
error (or a wrapped Error with context) instead of returning an empty array so
repository creation fails loudly; update the function that maps results (the
block referencing data, r.repository, r.profile_id, r.assignment_group_id,
r.assignment_groups?.name) to only run when no error was thrown.
In `@supabase/migrations/20260522130000_assignment-repo-config.sql`:
- Line 26: The new column submitted_via is currently unconstrained text, so
update the migration to enforce the closed set of allowed values by adding a
CHECK constraint (or use an ENUM) that restricts submitted_via to the documented
literals (e.g., the exact strings graders expect); locate the add column
submitted_via declaration and either create the column with the constraint or
add an ALTER TABLE ... ADD CONSTRAINT check_submitted_via CHECK (submitted_via
IN (...)) after it, and apply the same change for the other unconstrained
occurrence referenced around lines 94-95.
- Around line 80-81: The migration made public.submissions.repository and
public.submissions.sha nullable independently which allows inconsistent partial
states; add a table constraint on public.submissions (e.g.,
submissions_repo_sha_consistency) that enforces "(repository IS NULL AND sha IS
NULL) OR (repository IS NOT NULL AND sha IS NOT NULL)" so both columns are
either present or absent together; add this constraint after altering the
columns to nullable (or combine into the same migration) and choose a clear
constraint name like submissions_repo_sha_both_null_or_both_not_null.
- Around line 21-39: The new constraint assignments_no_protection_when_no_repo
requires protection columns to be unset when repo_mode = 'none', but the column
defaults (protect_block_force_push = true, protect_require_pull_request = false,
protect_required_reviewers = 0) make that impossible for
protect_block_force_push; fix by changing the defaults so a newly inserted row
with repo_mode = 'none' satisfies the constraint: set protect_block_force_push
default to false, keep or explicitly set protect_require_pull_request default to
false, and ensure protect_required_reviewers default is 0 (update the column
definitions for protect_block_force_push, protect_require_pull_request, and
protect_required_reviewers and then reapply the constraint
assignments_no_protection_when_no_repo).
In `@supabase/migrations/20260522130001_assignment-repo-config-enqueue.sql`:
- Around line 186-201: The migration silently no-ops when fork mode lacks a
source; update the validation around v_repo_mode and v_default_source so fork
paths fail fast: in the block that sets v_creation_method/v_default_source (and
the analogous block at lines 366-385) detect when v_creation_method = 'fork' (or
v_repo_mode in 'template_with_student_forks'/'fork_from_prior_assignment' entry
points) and v_default_source (or source_assignment_id) is NULL/empty and raise
an exception (use the same style as the existing template_repo exception)
instead of assigning a NULL v_default_source so the migration errors immediately
and surfaces the bad assignment config.
- Around line 14-20: Detect and fail fast when repo_mode =
'fork_from_prior_assignment' but the source assignment id is NULL: in
public.create_all_repos_for_assignment and public.create_repos_for_student add
an explicit check of v_source_assignment_id / r_source_assignment_id (or the
corresponding parameter variable) immediately after determining repo_mode and
before the lookup; if NULL raise an exception (or perform a RAISE EXCEPTION with
a clear message) instead of issuing a warning and continuing so enqueue work is
not silently skipped; do not add or drop any extra legacy
enqueue_github_create_repo overloads—the migration should only replace the new
16-arg overload as intended.
In `@supabase/migrations/20260522130002_assignment-no-repo-submission.sql`:
- Around line 72-99: The sequence that updates is_active, calculates v_ordinal,
and inserts a submission is vulnerable to races; obtain a transaction-scoped
lock before those three statements to serialize operations for the same
assignment scope. Acquire a per-assignment+scope advisory lock (use
pg_advisory_xact_lock) keyed by p_assignment_id and the scope identifier
(coalesce(v_assignment_group_id, v_profile_id)) at the start of the section that
runs the update/select/insert, then run the existing update, select into
v_ordinal and insert (which returns v_submission_id) while the lock is held so
duplicate ordinals and multiple active submissions cannot occur.
In `@tests/unit/branch-protection-rules.test.ts`:
- Line 11: Replace the relative import
"../../supabase/functions/_shared/branchProtection" with the project-root alias
form that starts with "`@/`"; update the import in the test (the one importing
branchProtection) to "`@/supabase/functions/_shared/branchProtection`" so it
follows the repo's `@/*` path-alias convention.
In `@tests/unit/handout-repo-strategy.test.ts`:
- Line 9: Update the import in tests/unit/handout-repo-strategy.test.ts to use
the project root alias instead of the relative path: replace the relative import
from "../../supabase/functions/_shared/handoutRepoStrategy" with the aliased
form "`@/supabase/functions/_shared/handoutRepoStrategy`" (and ensure the
referenced symbol names from handoutRepoStrategy continue to import correctly
and that your tsconfig/webpack path alias is configured if tests fail).
In `@tests/unit/repo-creation-strategy.test.ts`:
- Around line 10-12: Replace the two relative imports that reference
"../../supabase/functions/_shared/repoCreationStrategy" and
"../../supabase/functions/_shared/branchProtection" with project-root alias
imports using the `@/` prefix; update the import lines that bring in
repoCreationStrategy (symbols imported from repoCreationStrategy) and
DEFAULT_BRANCH_PROTECTION to import from
"`@/supabase/functions/_shared/repoCreationStrategy`" and
"`@/supabase/functions/_shared/branchProtection`" respectively so they follow the
`@/*` path-alias convention used across the codebase.
---
Outside diff comments:
In
`@app/course/`[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsx:
- Around line 1411-1421: The current JSX conditionally renders
StaffCommitHistory only when submission.repository_id and submission.repository
are present, which leaves upload/no-repo submissions with an empty UI; update
the render logic in layout.tsx around the StaffCommitHistory block so that when
submission.repository_id === null || submission.repository === null you either
(a) render a clear fallback UI element (e.g., a non-interactive message like "No
commit history available for upload submissions") or (b) render the same trigger
disabled with an explanatory tooltip/message; make the change by modifying the
conditional that references submission.repository_id and submission.repository
and/or by adding a small Fallback component or prop to StaffCommitHistory to
show the message instead of an empty dialog.
In `@supabase/functions/_shared/GitHubWrapper.ts`:
- Around line 916-970: The fallback path that handles "Name already exists on
this account" returns without applying branch protection, so add the same branch
protection sync that the fresh-create path runs: after enabling Actions (and
before returning heads.data.object.sha) call the existing branch-protection
logic using retryWithBackoff and octokit.request to set the branch protection
rules based on the branch_protection config (or invoke the helper used elsewhere
in this file if present), and capture/report any errors (set appropriate Sentry
tags like patch_existing_repo_settings_failed or similar) to keep branch rules
in sync.
---
Nitpick comments:
In `@supabase/functions/assignment-create-all-repos/index.ts`:
- Around line 10-17: Summary: The new shared imports use relative paths; change
them to the repo-root alias. Update the import lines that bring in
resolveRepoCreationStrategy, AssignmentForRepoCreation, SourceRepoRow,
StudentIdentity from "../_shared/repoCreationStrategy.ts" and
BranchProtectionConfig from "../_shared/branchProtection.ts" to use the project
alias (replace "../_shared/..." with "`@/` _shared/..." i.e. "`@/`
_shared/repoCreationStrategy.ts" and "`@/` _shared/branchProtection.ts") so the
file follows the repo-wide "`@/`..." import rule and matches other files' import
style.
In `@supabase/functions/assignment-create-handout-repo/index.ts`:
- Around line 5-13: The file imports shared utilities using relative paths
(applyBranchProtectionRuleset, createRepo, syncRepoPermissions,
updateAutograderWorkflowHash, assertUserIsInstructorOrServiceRole,
UserVisibleError, wrapRequestHandler, Database, resolveHandoutRepoAction,
HandoutSourceAssignment) — update those import specifiers from "../_shared/..."
to use the repo-root alias "`@/`..." so they follow the project rule (e.g.,
replace "../_shared/GitHubWrapper.ts" with "`@/` _shared/GitHubWrapper.ts" and
likewise for HandlerUtils.ts, SupabaseTypes.d.ts, and handoutRepoStrategy.ts);
ensure the import names remain unchanged and that any TypeScript path mapping
supports the "`@/`..." alias.
In `@supabase/functions/autograder-create-repos-for-student/index.ts`:
- Around line 9-14: Update the import paths to use the repo-root alias instead
of relative paths: change the imports that currently reference
"../_shared/repoCreationStrategy.ts" and "../_shared/branchProtection.ts" to use
"`@/` _shared/..." (e.g., "`@/` _shared/repoCreationStrategy.ts" and "`@/`
_shared/branchProtection.ts") while keeping the same exported symbols
(resolveRepoCreationStrategy, AssignmentForRepoCreation, SourceRepoRow,
BranchProtectionConfig) so the rest of the file continues to work with those
identifiers.
In `@tests/e2e/active-submission-gradebook-db.spec.ts`:
- Line 49: Replace the silent fallback assignment for assignmentSlug with a
fail-fast assertion: after calling insertAssignment (which sets
assignment_slug), assert that assignment.slug is non-null/defined (e.g., using
your test assertion helper or throw an Error) and then assign assignmentSlug =
assignment.slug; this ensures tests fail immediately when insertAssignment
returns a broken fixture instead of propagating a cryptic error into
getAssignmentGradebookColumn.
In `@tests/unit/no-repo-submission.test.ts`:
- Line 5: The import in tests/unit/no-repo-submission.test.ts uses a relative
path; replace the relative import with the project-root alias by importing
createNoRepoSubmission from "`@/lib/edgeFunctions`" instead of
"../../lib/edgeFunctions" so the test follows the repo's path-alias convention
and resolves from the project root (update the import that references
createNoRepoSubmission).
🪄 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: 9b887af9-09eb-48f4-8fb0-dd0e21897255
📒 Files selected for processing (26)
app/course/[course_id]/assignments/[assignment_id]/page.tsxapp/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsxapp/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/repo-analytics/page.tsxapp/course/[course_id]/manage/assignments/[assignment_id]/test/page.tsxapp/course/[course_id]/manage/assignments/new/form.tsxapp/course/[course_id]/manage/assignments/new/page.tsxlib/edgeFunctions.tssupabase/functions/_shared/GitHubAsyncTypes.tssupabase/functions/_shared/GitHubWrapper.tssupabase/functions/_shared/SupabaseTypes.d.tssupabase/functions/_shared/branchProtection.tssupabase/functions/_shared/handoutRepoStrategy.tssupabase/functions/_shared/repoCreationStrategy.tssupabase/functions/assignment-create-all-repos/index.tssupabase/functions/assignment-create-handout-repo/index.tssupabase/functions/autograder-create-repos-for-student/index.tssupabase/functions/github-async-worker/index.tssupabase/migrations/20260522130000_assignment-repo-config.sqlsupabase/migrations/20260522130001_assignment-repo-config-enqueue.sqlsupabase/migrations/20260522130002_assignment-no-repo-submission.sqltests/e2e/active-submission-gradebook-db.spec.tstests/unit/branch-protection-rules.test.tstests/unit/handout-repo-strategy.test.tstests/unit/no-repo-submission.test.tstests/unit/repo-creation-strategy.test.tsutils/supabase/SupabaseTypes.d.ts
- Toast wording in new-assignment page reflects no-repo modes - createNoRepoSubmission validates that the RPC returned a number - syncRepoPermissions revokes the students-team grant when student permission is no longer desired - Surface source-repo lookup errors in assignment-create-all-repos and autograder-create-repos-for-student so missing_source isn't conflated with a Supabase failure - assignment-create-handout-repo: persist template_repo only after the GitHub repo + permission sync succeed, and coalesce nullable protection columns to their defaults - Migrations: constrain submitted_via to known values, require submissions.repository and submissions.sha to be both-or-neither null, fail fast when fork mode has no source_assignment_id, and serialize create_no_repo_submission / create_manual_submission with per-scope advisory locks - Switch repo-strategy unit-test imports to the @/ alias Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
For repo_mode=template_with_student_forks and fork_from_prior_assignment
the student repo IS a GitHub fork of an upstream (the handout, or the
student's prior-assignment repo). Syncing those via the existing
template_pr flow round-trips the entire diff through a generated PR —
one API call to GitHub's native /merge-upstream endpoint replaces all of
it.
* queue_repository_syncs now reads a.repo_mode and resolves the
matching upstream_repo_full_name (per-student for mode 3, the
handout for mode 2) when enqueuing.
* github-async-worker dispatches sync_repo_to_handout messages with
sync_strategy='fork_merge_upstream' through a new
github.mergeForkUpstream helper. On dirty-branch or
no-longer-a-fork it falls back to the template_pr path so students
can still resolve conflicts via PR review.
* mergeForkUpstream pre-flights the repo's actual GitHub-tracked
parent before merging, so a rewired upstream surfaces as a
fallback instead of a silent wrong-source merge.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
While building E2E tests for the four new repo modes, surfaced and fixed
six real bugs:
1. submitted_via column added to assignments instead of submissions in
migration 20260522130000. Both create_no_repo_submission and
create_manual_submission inserted into submissions(submitted_via),
so both RPCs would have failed at runtime. Moved the column +
constraint to submissions and regenerated SupabaseTypes.
2. app/.../new/page.tsx only coerced protect_* fields to defaults for
repo_mode='none', missing 'no_submission'. With the default
protect_block_force_push=true, creating a no_submission assignment
would violate assignments_no_protection_when_no_repo. Now uses an
isNoRepo flag covering both modes; also nulls template_repo.
3. app/.../[assignment_id]/edit/page.tsx handed form values straight to
refineCore.onFinish with no coercion when the user flipped repo_mode
to a no-repo mode. Same constraint violation as #2. onFinish now
zeroes protect_* + nulls template_repo for no-repo modes, and nulls
source_assignment_id when flipping out of fork_from_prior_assignment.
4. assignment-create-handout-repo called applyBranchProtectionRuleset
twice on the happy path (once via createRepo, once again as a "safety
net"). The "already-exists" branch of createRepo did not call it at
all — that was the gap the safety net was masking. Moved the call
into createRepo's already-exists branch and dropped the redundant
outer call so it now fires exactly once per repo create.
5. @refinedev/supabase emits a malformed PostgREST filter for
operator: "nin" (omits the value-list parens). The source-assignment
picker in the form used "nin" and was completely broken in
production — the dropdown only ever showed the empty "Select an
assignment..." option. Switched to client-side filtering.
6. (pre-existing, not fixed) ManageAssignmentNav.tsx renders {children}
twice for responsive variants, double-mounting the AssignmentForm
and breaking RHF's ref binding for UI-driven edits. Documented in
the form-config E2E; tests scope to direct-DB assertions around it.
E2E additions (43 test cases across 4 new specs; 42 passing locally,
1 intentionally skipped):
* tests/e2e/no-repo-submission-flow.test.tsx (11) — create_no_repo_submission
RPC: happy path, deactivation, pre-release block, mode rejection, auth
gating, group flow, advisory-lock concurrency.
* tests/e2e/no-submission-manual-grading.test.tsx (19) — create_manual_submission
per-profile + per-group, idempotency, wrong-mode + non-instructor
rejection, XOR check, end-to-end grading on the stub submission.
* tests/e2e/assignment-repo-config-form.test.tsx (7) — mode toggling,
branch-protection enable/disable, reviewer-count validation, source-
picker filtering, round-trip persistence, validation errors.
* tests/e2e/fork-from-prior-assignment.test.tsx (5+1 skipped) — mode 2
handout creation, mode 3 inherit_from_source, fork_merge_upstream
worker path, constraint + trigger enforcement.
Test infrastructure:
* GitHubWrapper.ts: PAWTOGRADER_GITHUB_STUB=1 short-circuits createRepo,
applyBranchProtectionRuleset, syncRepoPermissions, mergeForkUpstream,
and updateAutograderWorkflowHash, recording each call into a new
public.e2e_github_calls table for assertion. PAWTOGRADER_GITHUB_STUB_MERGE_RESULT
overrides the mergeForkUpstream outcome.
* github-async-worker: the four e2e-ignore- short-circuits now bypass
only when the stub is OFF, so the worker actually exercises the stub
when it's enabled.
* tests/e2e/TestingUtils.insertAssignment extended with repo_mode,
source_assignment_id, protect_*; assignments_no_protection_when_no_repo
requires explicit false/false/0 for no-repo modes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI lint job rejected the previous commit because npm run format was not applied. No functional changes; pure whitespace. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new "Student Repositories" section on the assignment edit/create
form that lets instructors pick one of four modes: staff-only template
(current default), student-visible template with student forks, fork
from a prior assignment's per-student repos (#700), or no repository at
all (#699). The same UI exposes per-assignment GitHub branch-protection
rules (#698).
existing rows backfill to template_only_staff with block_force_push=true.
branch protection now flows through applyBranchProtectionRuleset which
builds rules from a BranchProtectionConfig (idempotent create / update /
delete) instead of always installing non_fast_forward.
autograder-create-repos-for-student route per-student repo creation
through new pure helpers (handoutRepoStrategy, repoCreationStrategy,
branchProtection) that are exercised by Jest unit tests with a mocked
GitHub layer, so the dispatch logic is covered without real network.
submissions; new create_no_repo_submission RPC + createNoRepoSubmission
client wrapper handle the upload flow.
Summary by CodeRabbit
New Features
Bug Fixes