Script and skill for setting up portal assignments [CLUE-654] - #2983
Script and skill for setting up portal assignments [CLUE-654]#2983scytacki wants to merge 1 commit into
Conversation
Testing CLUE against the real Firebase security rules requires a launch from a portal assignment carrying a portal token. `demo` and `qa` appMode both write to Firebase paths whose rules amount to `if isAuthed()`, so neither exercises them. Setting that assignment up by hand takes four coupled portal changes, each with a trap that fails quietly. scripts/setup-portal-assignment.ts creates or reuses the external activity, its offering on a class, a teacher report, and the OAuth redirect URI, then reads everything back. Every step is idempotent, so re-running after changing one flag reuses whatever already matches. `--dry-run` reports what would change without writing. scripts/lib/portal-api.ts holds the portal client: the JSON API takes a bearer token, but external reports, report attachment and OAuth clients exist only in the admin UI, so its forms have to be driven with a CSRF token and session cookie. It also supports the staging portal's own token, which the existing helpers did not. The traps the script encodes: - `append_auth_token` must be true, or the portal appends neither a token nor domain/domain_uid and CLUE silently falls back to preview mode, writing to the permissive /demo/ tree. A smoke test would pass while testing nothing. - The report URL needs its own firebaseEnv, or the teacher's CLUE reaches a different Firebase project than the students' and sees no work. - The OAuth client's redirect_uris is one shared whitespace-separated field rewritten whole, so it is appended to and verified rather than replaced. - Admin indexes are paginated. The production portal's CLUE OAuth client is on page two, so a single-page scan reports it does not exist. `PortalSession.readOnly` refuses any non-GET when `--dry-run` is set. The portal offers no read-only probe for "does an activity exist at this url" — the only one is `update_by_url`, a POST that also sets fields — so the guard lives in the session rather than relying on each call site to check. Docs corrected along the way. README.md and docs/deploy.md both described branch deploy paths without the issue-tracker stripping that the deploy action actually applies, so a Jira-named branch's URL was documented wrongly in two places; the stale `s3_deploy.sh` is now flagged as dead code. scripts/README.md gains the staging token and a note that the shared admin token is not a teacher, so endpoints like `classes/mine` return 403 by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #2983 +/- ##
===========================================
- Coverage 86.18% 70.53% -15.65%
===========================================
Files 989 984 -5
Lines 56618 56594 -24
Branches 14953 14951 -2
===========================================
- Hits 48794 39919 -8875
- Misses 7804 16639 +8835
- Partials 20 36 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
collaborative-learning
 |
||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
CLUE-654-portal-assignment-setup
|
| Run status |
|
| Run duration | 03m 36s |
| Commit |
|
| Committer | Scott Cytacki |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
4
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Pull request overview
Automates portal assignment setup for authenticated CLUE smoke testing against real Firebase rules.
Changes:
- Adds an idempotent portal setup script and API client.
- Adds setup guidance and read-only verification instructions.
- Corrects deployed branch-path documentation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/setup-portal-assignment.ts |
Creates activities, offerings, reports, and redirects. |
scripts/lib/portal-api.ts |
Adds portal API and admin-form support. |
scripts/README.md |
Documents tokens and assignment setup. |
README.md |
Corrects branch deployment paths. |
docs/deploy.md |
Documents branch-name stripping rules. |
.claude/skills/setting-up-portal-assignments/SKILL.md |
Adds portal setup guidance. |
.claude/skills/setting-up-portal-assignments/testing.md |
Adds read-only skill verification steps. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The portal and the Firebase project are independent choices, but pairing them is | ||
| // almost always what is wanted: a staging portal launch that wrote to production | ||
| // Firebase would be testing the rules of a project the assignment does not belong to. | ||
| firebaseEnv: raw["firebase-env"] ?? portal, |
There was a problem hiding this comment.
Confirmed — validProjects in src/lib/firebase-config.ts is ["staging", "production"] and currentEnvironment() falls back to production for anything else, so --firebase-env qa would have pointed a staging run at the production database while naming the resource ", qa FB". --firebase-env is now validated against a kFirebaseEnvs constant that names that source, and since the value is always one of the two, IOptions.firebaseEnv is no longer optional and the env && env !== "production" guards collapsed into one firebaseLabel() helper.
| const raw: Record<string, string> = {}; | ||
| const flags = new Set<string>(); | ||
| for (let i = 0; i < argv.length; i++) { | ||
| const arg = argv[i]; | ||
| if (!arg.startsWith("--")) usage(`Unexpected argument "${arg}"`); | ||
| const key = arg.slice(2); | ||
| if (key === "help" || key.startsWith("no-") || key === "dry-run") { | ||
| flags.add(key); | ||
| } else { | ||
| const value = argv[++i]; | ||
| if (value === undefined) usage(`Missing value for --${key}`); | ||
| raw[key] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed. The parser matched on shape (key.startsWith("no-")) rather than on known names, so --no-reprot became an inert flag and --activty-id 5 swallowed its value. Options are now matched against explicit kValueOptions / kFlagOptions lists and anything else exits with Unknown option "--...".
| activityId: raw["activity-id"] ? Number(raw["activity-id"]) : undefined, | ||
| oauthClientId: raw["oauth-client-id"] ? Number(raw["oauth-client-id"]) : undefined, |
There was a problem hiding this comment.
Fixed. Both paths were as described: activityId NaN is falsy so ensureActivity treated it as absent, and oauthClientId NaN survives ?? but fails the withReport && clientId guard, silently skipping the report and redirect. Both now go through a parseId() helper that rejects anything that is not a positive safe integer.
| }); | ||
| return true; | ||
| } catch (error) { | ||
| if (error instanceof PortalError && error.status === 500) return false; |
There was a problem hiding this comment.
The risk is real but the suggested remedy is not available. In the portal (rigse), update_by_url does ExternalActivity.where(url:).first then authorize on the result; for a miss that is authorize nil, which raises Pundit::NotDefinedError — not a subclass of NotAuthorizedError, so it is unrescued. Both staging and production set consider_all_requests_local = false, so the body is the generic 500 page, identical to the one any other server-side failure renders. There is no specific nil-record response to match on.
So I addressed the failure mode instead of the signal:
claimActivityByUrlnow retries once on a 500 before concluding "absent". A missing activity 500s every time; a transient failure usually does not repeat.- Before creating,
ensureActivitydoes one more read-only search and refuses to create if it finds an activity at that URL, pointing the user at--activity-id.ExternalActivity.createhas no URL uniqueness constraint, so a duplicate was the concrete bad outcome and this is the cheapest guard against it.
Neither is a proof of absence, and the comment now says so rather than implying the 500 is diagnostic.
| const updated = `${current.replace(/\s*$/, "")}\n${redirectUri}\n`; | ||
| await portal.submitForm( | ||
| `/admin/clients/${clientId}/edit`, | ||
| `/admin/clients/${clientId}`, | ||
| { "client[redirect_uris]": updated }, |
There was a problem hiding this comment.
The race is real, but neither remedy exists on the portal side. redirect_uris is a single text column on clients (rails/db/schema.rb), there is no append endpoint — only the admin PUT that rewrites the field whole — and there is no lock_version column anywhere in the portal schema, so ActiveRecord optimistic locking is not available either. The portal's own admin UI has the identical race when two admins edit this client.
What the code already does cover is the more damaging half: after the write it re-reads the field and throws if any URI from the pre-write snapshot is gone, and it refuses to write at all if the parsed field does not look like a list of URIs. The uncovered case is a URI added by someone else inside the read-write window of a manually-run setup script. Given no portal-side primitive to close it, I am leaving it rather than adding a retry loop that could not detect the conflict anyway. Happy to file a portal ticket for an append endpoint if that seems worth it.
Why
Most CLUE tests run in
demoorqaappMode, whose Firebase paths are governed by rules amounting toif isAuthed(). Only a launch from a portal assignment carrying a portal token exercises the realauthed/<portal>rules — which is how release smoke tests are meant to work.Setting that up by hand takes four coupled portal changes, several of which fail quietly rather than loudly. This automates it, and documents the parts a person still has to decide.
What's here
scripts/setup-portal-assignment.ts— creates or reuses the external activity, its offering on a class, a teacher report, and the OAuth redirect URI, then reads everything back and prints what a student and a teacher will each launch. Idempotent throughout;--dry-runwrites nothing.cd scripts npx tsx setup-portal-assignment.ts \ --clue-path version/v7.5.0 --class-id 111 --unit seismic --problem 1.1 --dry-run--clue-pathtakes the deployed path (version/<tag>orbranch/<name>) so the URL that gets built is visible in the command rather than inferred.scripts/lib/portal-api.ts— the portal client. The JSON API accepts a bearer token, but external reports, report attachment, and OAuth clients exist only in the admin UI, so its forms are driven with a CSRF token and session cookie. Also adds support for the staging portal's own token, which the existingscripts/libhelpers didn't have..claude/skills/setting-up-portal-assignments/— a skill covering the decisions the script can't make: which teacher and class, which unit, which problem, and what the deployed CLUE path actually is. Plustesting.md, a read-only procedure for verifying the skill after editing it.Doc corrections —
README.mdanddocs/deploy.mdboth described branch deploy paths without the issue-tracker stripping the deploy action applies, so a Jira-named branch's URL was documented wrongly in two places.scripts/README.mdgains the staging token and a note that the shared admin token is not a teacher.Traps encoded in the script
Reviewers may find these the most interesting part, since each one produces a plausible-looking result rather than an error:
append_auth_tokenmust be true. Otherwise the portal appends neither a token nordomain/domain_uid, CLUE falls back to preview mode, and every write lands in the permissive/demo/tree — the smoke test passes while testing none of the rules it exists for.firebaseEnv. Otherwise the teacher's CLUE reaches a different Firebase project than the students' and shows an empty report.redirect_urisis one shared whitespace-separated field, rewritten whole. The script appends and then verifies nothing was dropped, so a parsing mistake can't silently replace every other deployment's URI.On
--dry-runPortalSession.readOnlyrefuses any non-GET when the flag is set, rather than each call site checkingoptions.dryRun. That's deliberate: the portal offers no read-only probe for "does an activity exist at this url" — the only one isupdate_by_url, a POST that also sets fields. An earlier revision used it during dry runs and quietly clearedappend_auth_tokenon a real activity. Reasoning about which calls write is exactly the method that let that through, so the session enforces it instead.The consequence is visible in the output: a dry run cannot tell whether it would create or reuse an activity, and says so rather than guessing.
Testing
testing.md— three runs, all reaching the correct command and dry-run output. A baseline without the skill missed the class identification every time, which is the item the skill leads with.tsc --noEmitand eslint clean on both new files. Notescripts/is outside the repo's lint globs, so this was run against them directly.🤖 Generated with Claude Code