CLUE-610: Deferred open and create-time ownership rules

[scytacki]

Built on #2949, now merged; this PR targets master.

CLUE-610's remaining half: the things that should not be left open once a real curriculum unit turns the class-wide document on.

Opening a unit no longer waits on the class-wide document

getOrCreateCanonicalDocument splits into a resolver — which converges the whole class onto one document per slot — and a thin opener over it. A declared class-wide document is now resolved at unit load but not opened, so the fast path costs one pointer read instead of a second metadata read, an RTDB fetch, a DocumentModel, and a history subscription for every student on every load. That was measured at ~700ms.

Group documents are unchanged: getOrCreateGroupDocument still opens, and because the resolver hands back the metadata it already holds on the legacy and create paths, opening one costs no extra read.

The document is opened on demand instead — from Sort Work, or by guaranteeInitialDocuments after a reload with it as the primary document. That second path was gated on groupDocumentsEnabled, which a unit can legitimately leave off while declaring classWideDocuments; the gate now covers both, so such a unit no longer loses its primary document on reload.

A document can no longer be created under someone else's name

isValidDocumentCreateRequest constrained neither uid nor concurrent, so a class member could create a document stamped with a classmate's id — it would then appear under that classmate's name in Sort Work — or one already marked class-shared, which hands the whole class read and write on its history.

The obvious fix does not apply, because documents several people edit together are deliberately owned by a synthetic id rather than a person. So the rule accepts three shapes, each corroborated by the caller's token or by fields the document itself carries: the caller; class_<class_hash> of the caller's own class; or group_<offeringId>_<groupId> agreeing with the document's own offeringId and groupId. concurrent: true at creation requires one of the two synthetic owners.

Two residuals are deliberate, and are recorded in docs/document-axes/README.md and in the rules comments rather than left implicit:

• Nothing proves the caller is a member of the group they name. Group membership lives in the Realtime Database and the token carries no group claim, so a student can still create a document owned by another group in their own offering.
• Any class member may mint documents owned by their own class. The class identity is corroborated; the authority to mint under it is not. That is inherent — minting is how a class converges on one document.

concurrent also remains settable on an existing type: "group" document, a transitional allowance the backfills need. CLUE-612 closes it once CLUE-604's migration has drained.

Deploying this

The rules must go out a release ahead of the app, per the usual pattern. Group and class-wide document creation is expected to fail in the window between the two — the canonical pointer paths in #2949 move — and neither is in use in a released unit. Nothing else may break, which is what the new "document shapes the deployed client creates" tests hold: one create per type the current client writes, all still permitted. Deliberately no keys().hasOnly(...) allowlist on the create — that would have to enumerate every shape the deployed client writes and is the change most likely to break unrelated document types.

Verification

npm test, check:types, lint:build, and the firebase-test emulator suite all green.

The reload case above is now covered by cypress/e2e/functional/document_tests/class_wide_document_spec.js, on a new demo/units/qa-class-wide unit that declares a class-wide document with groupDocumentsEnabled off. The spec closes the document in Sort Work before reloading: left open, Sort Work re-opens it on reload and the assertion passes even with the gate narrowed back to groupDocumentsEnabled. Confirmed by reverting the gate and watching it go red.

That suite of rules tests, which the deploy argument above leans on, was not run by any workflow. #2976 adds it to CI.

Confirmed in Chrome against live Firestore on demo/units/qa: the document is created but absent from stores.documents on both the create and fast paths; Sort Work shows it under all four filters and opens it on demand with an Edit button; a reload with it as the primary document restores it with groupDocumentsEnabled off (checked with the Problem tab active, so Sort Work could not be what re-opened it); group documents open unchanged.

Worth knowing for anyone repeating that: unit=qa loads the unit from the remote curriculum repo, which has no class-wide slot, so the feature silently isn't there. The local unit has to be named outright — unit=http://localhost:<port>/demo/units/qa/content.json.

One nuance the manual run turned up: expanding the "Whole Class" section in Sort Work opens the document, because rendering the thumbnail fetches it. Still on demand, still off the unit-load path, but a lower bar than an explicit open.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.53%. Comparing base (5fa7976) to head (48ae4c2).
⚠️ Report is 81 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2951      +/-   ##
==========================================
- Coverage   86.10%   85.53%   -0.58%     
==========================================
  Files         983      982       -1     
  Lines       56122    56120       -2     
  Branches    14807    14809       +2     
==========================================
- Hits        48325    48000     -325     
- Misses       7777     8102     +325     
+ Partials       20       18       -2     

Flag	Coverage Δ
cypress-regression	70.22% <57.14%> (-1.02%)	⬇️
cypress-smoke	41.11% <50.00%> (-0.35%)	⬇️
jest	56.99% <92.85%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cypress]

collaborative-learning    Run #20058

Run Properties:   Passed #20058  •  48ae4c2374: refactor: keep resolveClassWideDocument public [CLUE-610]

Project	collaborative-learning
Branch Review	CLUE-610-deferred-open-and-create-rules
Run status	Passed #20058
Run duration	03m 39s
Commit	48ae4c2374: refactor: keep resolveClassWideDocument public [CLUE-610]
Committer	Scott Cytacki
View all properties for this run ↗︎

---

Test results
Failures	0
Flaky	0
Pending	0
Skipped	0
Passing	4
View all changes introduced in this branch ↗︎

[Copilot]

Pull request overview

Defers opening class-wide documents until needed and tightens document creation ownership rules.

Changes:

• Separates canonical document resolution from opening.
• Restores deferred class-wide primary documents on reload.
• Adds ownership/concurrency validation, tests, and documentation.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/models/document/document-utils.ts	Updates documentation reference.
src/models/document/document-axes.ts	Updates axes documentation reference.
src/lib/db.ts	Introduces deferred canonical document opening.
src/lib/db.test.ts	Tests resolver and opening behavior.
src/components/document/document-workspace.tsx	Restores class-wide primary documents.
firestore.rules	Validates document owners and concurrent creation.
firebase-test/src/documents-rules.test.ts	Tests document creation rules and deployed shapes.
docs/superpowers/specs/2026-08-07-clue-610-deferred-open-and-create-rules-design.md	Documents the design and rollout.
docs/document-axes/README.md	Updates owner-axis status and links.
docs/document-axes/axes-current-state.md	Documents current rule enforcement and residuals.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[kswenson]

👍 Looks good -- the following review, developed in conjunction with Claude Code 🤖, identified a few issues worthy of consideration.

PR Review Summary

Changes: 13 files, +600 / −80 lines

What it does

CLUE-610's second half — the things that shouldn't be left as-is once a real curriculum unit
turns the class-wide document on. Two independent changes share one gate:

1. Opening a unit no longer waits on the class-wide document.
   getOrCreateCanonicalDocument splits into a resolver (resolveCanonicalDocument, which
   converges the class onto one document per slot) and a thin opener over it. A declared
   class-wide document is resolved at unit load but not opened, trading a second metadata read,
   an RTDB fetch, a DocumentModel and a history subscription — measured at ~700ms per student
   per load — for a single pointer read. It is opened on demand instead.
2. A document can no longer be created under someone else's name.
   isValidDocumentCreateRequest previously constrained neither uid nor concurrent. It now
   admits three owner shapes — the caller, class_<class_hash> of the caller's own class, or
   group_<offeringId>_<groupId> agreeing with both the caller's token and the document's own
   fields — and permits concurrent: true at creation only under one of the two synthetic owners.

A third, smaller fix rides along: guaranteeInitialDocuments was gated on groupDocumentsEnabled,
which a unit may legitimately leave off while declaring classWideDocuments, so such a unit lost
its primary document on reload. The gate now covers both.

The approach

The split is the right shape. Separating convergence from opening is the real insight here —
the get-or-create has to happen at unit load because that is what makes the whole class agree on
one document, but opening it does not. Returning firestoreMetadata from the paths that already
hold it (db.ts:936, db.ts:969) keeps getOrCreateGroupDocument at exactly its previous read
count, so the deferral costs group documents nothing.

On the rules side, the choice not to constrain type (firestore.rules:232-236) initially looks
like a gap and is in fact correct: class-wide and group documents both store type: "group",
and target-architecture.md explicitly names branching on type as the anti-pattern the axes
work is retiring. Tying the owner check to type would buy no boundary and would have to be
reverted under a coordinated rules deploy when CLUE-604 lands.

Both residuals are recorded rather than assumed away, in the rules comments and in
axes-current-state.md. That is the right instinct, and the reason the review below is mostly
about the description of one residual rather than its existence.

Assessment

This is solid work and I'd approve it. I ran a five-lens find pass and then put every
candidate finding in front of an adversarial verifier, and the security findings all dissolved
in the same direction: the attacks are real but pre-date the PR, and this change strictly
narrows them. Worth stating plainly, because it is the core question on a rules change:

• Pre-PR, isValidDocumentCreateRequest had no uid constraint at all — any class member
  could mint a document under any uid whatsoever, including a classmate's real
  platform_user_id, already stamped concurrent: true. The PR inserts two conjuncts into an
  AND chain and touches nothing else in the file. It is monotonically restrictive: it can only
  deny more.
• The post-PR residual (own offering, any group in it) is a strict subset of what was
  reachable before.

I checked the deploy story too, since that is where a rules-ahead-of-app change usually breaks:
every client create path funnels through getDocumentOwner(), which only ever returns the
caller's uid or one of the two synthetic owners — exactly the three shapes the rule admits. The
foreign-uid writes I went looking for (exemplars, comment-created documents, published
supports) all turn out to be admin-credential writes from Cloud Functions, which bypass
firestore.rules entirely, or go to a different collection with its own rule. So the "nothing
else may break" claim holds for the shapes I could find.

Verification I ran locally

Check	Result
npm run check:types	clean
npx jest src/lib/db.test.ts	43/43 pass
firebase-test rules suite (emulator)	10 suites, 430 passed, 2 skipped
CI — Build, Jest, 10 regression shards, smoke	all pass

One test-quality result worth calling out: a verifier mutation-tested the new rules by disabling
documentOwnerOk() and concurrentCreateOk() in turn. Each mutation turns the expected tests
red, so the new clauses have genuine regression-detecting coverage rather than tests that merely
happen to pass. That is better than most rules test suites manage.

Issues

1. The reopen-primary fix has no automated coverage at any layer. — medium
src/components/document/document-workspace.tsx:204
The || classWideDocuments?.length gate is a real bug fix (a unit declaring classWideDocuments
with groupDocumentsEnabled off silently lost its primary document on reload), and it is
verified only by hand in Chrome, as the PR body says. There is no document-workspace test file
in the repo at all, guaranteeInitialDocuments is reachable only from componentDidMount, and
no cypress spec references classWideDocuments. So the next refactor of this method regresses
the fix silently. Given the rest of the PR is unusually well tested, this one stands out.
Failure scenario: someone later simplifies the gate back to groupDocumentsEnabled, or changes
the primaryDocMetadata.type === GroupDocument check; npm test and the full regression suite
both stay green, and class-wide-only units lose their primary document on reload again.

2. The firebase-test rules suite is not run by any CI workflow. — medium (repo-level)
grep -rln firebase-test .github/workflows/ returns nothing. The 145 new lines of rules tests —
the stated safety net for shipping these rules a release ahead of the app — only ever run when
someone remembers to run them locally. This isn't a defect introduced by the PR, but this PR is
the one whose deploy argument rests on those tests, so it's the natural moment to raise it.
Two frictions make "remember to run it locally" worse than it sounds: firebase-test/node_modules
isn't installed by the root npm install, and npm run test:exec now fails outright —
firebase-tools 15 requires Java 21 and the documented requirement in CLAUDE.md is just "Java".
I had to fall back to npx firebase-tools@13 to run it at all.

3. The accepted group residual understates its consequence. — low/medium, documentation only
firestore.rules:196-201, docs/document-axes/axes-current-state.md:127-132
Both stop at "a student can still create a document owned by another group in their own
offering," which reads like a stray object nobody sees. The actual reachable consequence is
larger, because two unchanged rules compose with it: canonicalPointerCreatable
(firestore.rules:755) checks only class_hash and never group membership, and pointers are
immutable (allow update, delete: if false) — so the same caller can win and permanently hold
that group's canonical slot, which every real group member then silently converges onto via the
fast path. isConcurrentClassDocument (firestore.rules:613) then grants history read/write to
the whole class, not the group. Since the residual is being deliberately accepted and written
down, the write-up should say what is being accepted. A sentence or two, not a code change.

4. Jira keys land in firestore.rules for the first time. — low
firestore.rules:119 (CLUE-612), :120 (CLUE-604), :125 (CLUE-612)
This repo is public; the Jira instance behind these keys is not. A reader outside Concord hits
That deletion is CLUE-612 and has no way to find out what CLUE-612 says, which is a shame here
because the surrounding comments are otherwise unusually self-contained — they explain the
transitional allowance, why it exists, and what has to drain before it can go, all without
requiring the ticket. The keys are the one part a public reader can't follow.

firestore.rules had zero Jira keys before this PR, so this is a new precedent in that file
rather than a continuation. (The wider repo does have ~62 in src/, so this isn't a rule anyone
has been enforcing — hence low severity.) Suggestion: keep the explanatory prose exactly as it is,
and move the bare ticket references to the PR description, or phrase them so the sentence survives
without the key — "once the backfill migration has drained" rather than "once CLUE-604 has drained."

Pre-existing, worth a follow-up ticket (not this PR's to fix)

Three separate verifiers independently landed on the same unchanged hole, which suggests it's
worth capturing somewhere:

canonicalPointerCreatable lets any class member permanently squat any canonical slot.
firestore.rules:755-758 requires only keys().hasAll(["documentKey", "createdAt", "createdBy"])
— presence, not type or non-emptiness. Compare the neighboring isValidCommentTagRequest, which
does check label is string && label.size() > 0. Because pointers are immutable and the resolver's
fast path returns whatever the pointer names, a single hand-crafted write of
{documentKey: "", ...} into a slot permanently kills that slot for the whole class — no document
required. Identical before and after this PR. Adding documentKey is string && documentKey.size() > 0
would be a small, safe hardening, and would sit naturally alongside CLUE-612.

Nits — take or leave

• resolveClassWideDocument is public, but its return value has no production consumer.
  createDeclaredClassWideDocuments discards it, and Sort Work finds these documents through an
  independent metadata query, never through the pointer key. That appears to be intentional (the
  caller's only job is convergence), but public on a method whose result nobody reads invites the
  next reader to go looking for the consumer. Worth a word, or private.
• concurrent: null at create is rejected where an omitted or false value is accepted —
  data.get("concurrent", false) substitutes the default only for an absent key. Unreachable
  from the app, which never stamps an explicit null; noting it only because a future script could.
• Units with classWideDocuments and groupDocumentsEnabled off now pay one extra
  findFirestoreMetadata read per mount whenever any primary document is persisted, even an
  ordinary personal document. That's inherent to the fix — you can't know whether the primary
  document is a group document without reading its metadata — so I don't think it's avoidable,
  just worth knowing it's there in a PR that's otherwise about removing a read.
• The dead case SupportPublication: at src/lib/db.ts:770 cost me a verifier run. It has had
  no caller since supports moved to mcsupports, and it reads as a live client create path. Not
  this PR's debt, but a one-line comment or a deletion would save the next reader the same trip.

Findings that did NOT survive verification

Recorded so they don't get re-litigated later — each was checked against the pre-PR rules at
merge-base 5fa79768d and refuted:

• "A classmate can capture another group's canonical document" — mechanically true, entirely
  pre-existing; the PR strictly narrows the create side. Rolled into the documentation point (3)
  and the follow-up ticket above.
• "The rules don't tie type to the owner shape" — deliberate and correct; class-wide documents
  are type: "group", so the tie would close nothing and would fight the axes migration.
• "The classmate-plus-concurrent test passes for the wrong reason" — the create is denied
  independently by both clauses; mutation testing showed concurrentCreateOk is properly isolated
  by its two sibling tests. A legitimate belt-and-braces deny test.
• "SupportPublication is missing from the deployed-shapes list" — supports are admin-written to
  mcsupports by a Cloud Function and already covered in supports-rules.test.ts.
• "The confirmed exists comment overstates what was verified" — the comment says "without
  fetching its metadata" and calls the property an invariant in the same breath; the invariant is
  genuinely established by create-then-claim plus pointerConfirmsCanonical plus the canonical-delete ban.
• "Deferring the open makes a corrupt pointer fail silently" — pre-PR it landed in the same
  fire-and-forget .catch with a bare console.error. Real delta is one lost log line, in a state
  no app writer can produce.

[scytacki]

Responses below, in the order raised. Four commits, plus one docs commit updating the design spec's Testing section.

1. The reopen-primary fix has no automated coverage — fixed

cypress/e2e/functional/document_tests/class_wide_document_spec.js, on a new demo/units/qa-class-wide unit that declares a class-wide document with groupDocumentsEnabled off — the combination the widened gate exists for. A student edits the class-wide document from Sort Work, reloads, and the workspace still shows it.

The part worth reporting is that my first version of this test passed with the fix reverted. Sort Work re-opens whichever document it had open, so it was loading the document on reload regardless of what guaranteeInitialDocuments did — exactly the "tests that merely happen to pass" failure your mutation-testing note was guarding against. Closing the document in Sort Work before reloading is what gives it teeth:

gate	result
groupDocumentsEnabled || classWideDocuments?.length	passes, ~11s local / 16s in CI, no retries
reverted to groupDocumentsEnabled	fails — the primary workspace renders empty

The unit is a single file with inline problem sections, so no new section-file tree. It also asserts the "Whole Class" section holds exactly one document, which has held across every run — the pointer converges, and repeated runs don't accumulate throwaway documents in the QA partition.

2. firebase-test is not run by any CI workflow — fixed, in #2976

Separate PR, since you're right that it isn't a defect this one introduced, and it's judged on its own terms.

The Java requirement is real and hard — firebase-tools 15 throws rather than warning:

// firebase-tools/lib/emulator/controller.js
if ((await commandUtils.checkJavaMajorVersion()) < commandUtils_1.MIN_SUPPORTED_JAVA_MAJOR_VERSION) {
  throw new error_1.FirebaseError(commandUtils_1.JAVA_DEPRECATION_WARNING);
}

But it costs nothing in CI. ubuntu-latest ships Java 8, 11, 17 (default), 21 and 25 preinstalled, so setup-java picks 21 out of the image's toolcache:

Resolved Java 21.0.12+8 from tool-cache
Setting Java 21.0.12+8 as the default

Whole job: 1m30s.

The thing that actually kept the suite out of CI was neither Java nor the install. test:exec invokes a bare firebase, and no package.json in the repo depends on firebase-tools — so it only ever worked on a machine with a global install, which a runner doesn't have. Pinning firebase-tools as a devDependency of firebase-test fixes that and makes the Java floor an explicit, versioned decision rather than a property of whoever's laptop is running it. A plain npm ci in that directory becomes the whole setup, for CI and for a new contributor both.

The job pings Slack on a master failure alongside jest and cypress, but deliberately does not gate s3-deploy — firestore.rules deploys separately, so an emulator hiccup shouldn't hold up an app deploy.

CLAUDE.md's "Node.js 16.x and Java" note is corrected there too, along with a stale engines: {node: "16"} in firebase-test that made every install print EBADENGINE under the Node 20 the rest of the repo uses.

3. The accepted group residual understates its consequence — fixed

Checked all three composing rules before writing anything down, and they hold: canonicalPointerCreatable (firestore.rules:754-757) tests only request.auth.token.class_hash == classId plus key presence; the recursive {slotPath=**} block sets allow update, delete: if false; and isConcurrentClassDocument keys on context_id, i.e. the class, not the group.

Both the rules comment and axes-current-state.md now say that the same caller can claim that group's canonical slot and hold it permanently, that every real member converges onto it through the resolver's fast path, and that its history is readable and writable by the whole class rather than the group.

4. Jira keys in firestore.rules — fixed

All three removed, prose kept. "until the CLUE-604 migration has drained" is now "until the backfill this function exists for has drained in every environment," and the trailing "That deletion is CLUE-612" is simply gone — the paragraph already says the function should be deleted and names the condition.

Pre-existing follow-up: canonicalPointerCreatable slot squatting

Not addressed here, and I agree it shouldn't be. It's identical before and after, and documentKey is string && documentKey.size() > 0 is a one-line hardening that reads naturally alongside the concurrent-immutability work rather than bolted onto this PR. Worth its own ticket.

Nits

• resolveClassWideDocument stays public, with a comment instead. I tried private first, but it only moved the problem: the two tests that exercise the resolver directly then had to reach it through (db as any), trading real type checking in the tests for a visibility keyword. The point behind your nit — that public on a method whose return value nobody reads sends the next reader hunting for a consumer that doesn't exist — is answered by a line saying the key is returned because it names what the class converged on, not because a production caller reads it.
• Dead case SupportPublication: now carries a comment saying supports are admin-written to mcsupports by a Cloud Function, so the next reader doesn't spend a pass on it.
• concurrent: null at create — left as is. data.get("concurrent", false) substituting only for an absent key is the behaviour I'd want anyway: an explicit null is not a shape the app produces, and admitting it would mean widening the rule to accept a value that means nothing. Recorded here rather than changed.
• The extra findFirestoreMetadata read per mount — left as is, for the reason you gave: you can't know whether the primary document is a group document without reading its metadata. Real, unavoidable, and now at least paid only on units that declare one of the two features.

On the "did not survive verification" list — no disagreement with any of those, and the pre-PR comparison at the merge base is the right frame for the security ones. Nothing there needs an answer.