Skip to content

Backfill offeringId onto offering-contained documents [CLUE-643] - #2980

Open
scytacki wants to merge 28 commits into
masterfrom
CLUE-643-document-offering-id-backfill
Open

scytacki wants to merge 28 commits into
masterfrom
CLUE-643-document-offering-id-backfill

Conversation

@scytacki

@scytacki scytacki commented Aug 25, 2026

Copy link
Copy Markdown
Member

A repair script only — no product code changes, and nothing here has been run against a database in
apply mode.

The problem

isInClassUnitContainer (src/models/document/document-axes.ts) identifies a document's container by
the absence of offeringId. So an offering-contained document that has no offeringId on its
Firestore metadata reads as belonging to the class's copy of the unit — the wrong container axis.

71,399 documents across authed and demo are in that state. This script recovers the value from
the realtime database, where the document's own metadata node has carried it all along, and writes
it back.

What's here

scripts/metadata-repair/backfill-document-offering-id.ts  the sweep
scripts/lib/document-metadata-lookup.ts                   the shared realtime-database lookup
scripts/find-documents-missing-metadata.ts                a small census used while designing this

The sweep sits beside the other CLUE-643 repair scripts, and scripts/metadata-repair/README.md
now covers it as step 5 of the runbook. It uses those scripts' isRtdbAddressable and
resolveDatabaseUrl rather than its own copies.

Documents are scanned by collection group, one type at a time, using the containerType: "offering"
types from document-kinds.ts. Each candidate is classified into exactly one bucket, and only the
resolved bucket is written — update({ offeringId }), batched, with the written count
incremented only after a commit resolves. update rather than set with merge, so a document
deleted between the scan and the commit is not recreated holding only an offeringId. A batch that
fails with NOT_FOUND is retried one document at a time, and the deleted document is counted as
deletedDuringRun. A failed run prints its whole partial result, including written and the
per-type and per-space counts.

"publication" appears in the type list because that is the problem publication's stored value;
ProblemPublication in document-types.ts is the constant's name, not its value. Both "group" and
"axes" appear because the generic axes type is mid-rename, and accepting both is what lets this
sweep and backfill-group-document-axes.ts run in either order.

What the production census found (2026-08-20)

A full dry run scanned 112,900 documents in 4m24s.

root spaces docs resolved alreadySet noMetadataNode keyNotRtdbSafe classWide
authed 5 95,772 69,501 26,182 43 2* 44
demo 426 3,566 1,898 1,653 8 7
dev 3,631 4,473 588 3,793 92
qa 3,559 9,089 22 6,217 1,104 1,746
total 112,900 72,009 37,845 1,247 2 1,797

* Both were deleted on 2026-08-20, so a re-run reports zero. They were supportPublication rows from
2021 keyed 2.2 Initial Challenge Support 1 and 2.2 Support 1 — curriculum-authored supports whose
keys contain a ., which a realtime-database path forbids. Scanning all 146,105 documents in the
collection group confirmed they were the only two, and neither carried a unit, so they never
endangered isInClassUnitContainer in the first place.

A 99.93% recovery rate across real data, with 51 documents unrecoverable. That settles a question
the design deliberately left open: two further recovery sources were sketched (walking the offering
tree, and a fuzzy match against the portal API), and neither is worth building to rescue fifty-odd
documents. "Report and leave alone" is the policy, and it is what the script already does.

nodeWithoutOfferingId is 0 across all 112,900 documents. There is no population of realtime
nodes written without an offering; the only failure mode is the node being absent entirely.

Safety

Dry run by default, APPLY=1 to write, mode printed at startup. TYPES= limits the scan to a subset
of types and PAGE_SIZE= tunes the query page, so a large environment can be sampled before a full
sweep is committed to. The census prints from a finally block, so a run that dies still says what it
did.

Idempotency works differently here than in a create-only script: a second run reclassifies the
documents the first one wrote as alreadySet rather than rewriting them.

Requires a deployed index

The collection-group queries need the single-field COLLECTION_GROUP index on type, declared in
firestore.indexes.json. It was deployed to staging and production on 2026-08-13, so neither
environment needs anything done. A new environment would need it created before this script runs at
all — the first query fails outright without it. Diff against the deployed indexes before deploying:
an environment may carry indexes absent from the file, which a --force deploy would delete.

Sequencing

No app code, no rules change, no client behavior change, so this needs no drain, no deploy window and
no place in the release chain.

It does need to run after the two repairs in #2977. This script finds a document's
realtime-database node through its context_id, so a wrong one makes the document look unrecoverable;
and rows that do not exist yet cannot be scanned at all.

That predicts something worth checking when the repairs run: the 43 authed noMetadataNode
documents should shrink, because some of them are likely the documents whose context_id names the
wrong class. If the count does not move, the two populations are disjoint and that is worth knowing
too.

Against CLUE-604's axes backfill it is order-independent, because this script accepts either side of
that rename.

Testing

47 unit tests across 2 suites, against a mock Firestore and a mock realtime database. Worth a look
in review: the bucket classification table, the pagination boundary, the deleted-during-run retry,
and the keyNotRtdbSafe bucket — a key containing ., #, $, [, ], / or an ASCII control
character cannot appear in a realtime-database path, and a lookup on one throws rather than returning
empty.

Not in this PR

Running it. APPLY=1 needs a separate go-ahead, after the repairs in #2977 have been applied.

🤖 Generated with Claude Code

scytacki and others added 20 commits August 20, 2026 09:48
…CLUE-643]

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scan is ordered by document id, so two documents belonging to the same
user essentially never land in the same page and a per-user cache would hit
approximately never. Getting the clustering that would make caching pay off
means ordering by uid, which needs a new composite collection-group index.

Also record that the extracted lookup must avoid import.meta so Jest can
load it.

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

A transport error is not evidence that a document has no offering, so it gets
its own census bucket rather than joining the two "not found" ones. The
extracted lookup throws and each caller decides what to do.

Also pin the classification order: class-wide is decided before alreadySet, so
a class-wide document that wrongly carries an offeringId is still reported.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-643]

Adds the paginated collection-group scan that census-tallies every
offering-contained document by outcome, type, and space, with RTDB
lookups run concurrently per page. Nothing is written yet; dryRun is
honored and written stays 0 until a later task drains the resolved
lookups into batched writes.

Also adds a Jest moduleNameMapper rule stripping a relative import's
trailing `.js` extension so ts-jest's default resolver can find the
sibling `.ts` file. scripts/ is an ESM package (`"type": "module"`),
so its local imports use the `.js` extension per Node's ESM rules,
but ts-jest never transpiles that extension away for value imports,
and Jest's resolver treats an explicit extension as literal. This is
the first script under test that imports another script module as a
value rather than only a type, so the gap was previously unexercised.
]

Jest resolves only the extensionless specifier to the sibling .ts file, so the
tested script uses that form. tsx resolves either, verified against the module.
Reverts the moduleNameMapper entry, which rewrote every relative .js import in
the repo to fix a scripts-only resolution problem.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d report on crash [CLUE-643]

Applies the final code review's fix wave to backfill-document-offering-id.ts:
derive the Realtime Database URL from the service account instead of
hardcoding it, so the script cannot read one environment's offerings and
write them onto another's documents; count `written` only after a batch
commits so a failed run cannot over-report what landed; print the census
in a `finally` and log per-type progress so a crash still leaves a record
of how far the run got; and reject a non-string offeringId in the shared
RTDB lookup so a legacy numeric value can't silently fail the app's
string-typed Firestore query. Adds tests for the new type check, the
default dry-run behavior, and per-bucket accounting in the mixed-bucket
test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Corrects three claims that implementation made untrue: per-type queries are
not independently resumable, group documents are exempt from the uid
derivation rather than from repair, and the order-by uses the "__name__"
string so the module needs no runtime firebase-admin import.

Adds an "Operating the run" section for the properties that matter to whoever
points this at production data: the database URL is derived from the
credential rather than named, `written` counts committed writes so a dying run
under-reports rather than over-reports, a failed run still prints its census,
and there is no resume.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…E-643]

The two Firebase projects do not share an RTDB host pattern: production is
collaborative-learning-ec215.firebaseio.com, staging is
collaborative-learning-staging-default-rtdb.firebaseio.com. Deriving the URL
from the project id produced a wrong host for staging, so the script now looks
it up and fails loudly on an unknown project rather than guessing. DATABASE_URL
overrides.

Also correct the index claim, which a staging run disproved: the collection
group index on documents.type is declared in firestore.indexes.json, but that
file was reconciled against production, so each environment needs it actually
deployed. The first query fails outright without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nown [CLUE-643]

A staging census scanned 209 documents, of which 68 were qa and 46 dev. Those
roots are keyed by user id rather than portal, so they matched no space and
were labelled "unknown" — while being counted under buckets that say nothing is
wrong, because they all short-circuited as alreadySet or skippedClassWide
first. A partition document that actually needed repair would have reported as
an unrecognized path shape, which reads as an anomaly rather than scratch data.

They now get their own bucket, asked before every other question because it is
a matter of scope rather than a property of the document. unknownSpace is left
meaning what it says.

The space label now travels with the classification, so the path is parsed once
instead of once per document at each of two sites.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…LUE-643]

TYPES limits the scan to a subset, for a cheap first look before committing to
a full sweep; PAGE_SIZE tunes the query page. Both are parsed and validated
before anything connects, and an unrecognized type name throws rather than
scanning nothing — a typo would otherwise produce a clean empty census that
reads exactly like "this type has no problems". The run header now echoes
types, page size and mode, so what a run did is recoverable from its log.

Also correct the index claim again: the collection group index on documents.type
is deployed in neither staging nor production. It was added to the file
alongside backfill-group-document-axes.ts, which needs the same index and has
not been run. It is a prerequisite for the sweep in every environment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-643]

It was created in staging and production on 2026-08-13, and indexes persist, so
CLUE-604 inherits a satisfied prerequisite rather than a step to repeat. Kept as
a note because the symptom is otherwise baffling: without it the first query
fails outright, identically for both sweep scripts.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-643]

The qa and dev partitions are no longer skipped. Both stores derive their root
from the same getRootId, so the Firestore root id is also the RTDB root id; the
only missing piece was the portal segment, which sampling production settles as
"localhost" for dev and "qa" for qa. `test` takes an arbitrary portal and still
cannot be derived, so it reports as an unknown space rather than being guessed
at — production has no test documents.

A production census also found curriculum supports keyed like
"2.2 Initial Challenge Support 1". A "." is illegal in an RTDB path, so those
lookups throw and were counted as lookupError, which reads as transient when the
failure is permanent. They now get their own bucket, decided before the lookup.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
112,900 documents scanned. Across real data 71,399 need an offeringId and 53
cannot get one — a 99.93% recovery rate from the single source this script
uses, which settles the question the spec left open: do not build the
offering-tree or fuzzy portal sources.

Also records what the per-root split shows and the totals hide: 88% of the
unrecoverable residue is qa, whose RTDB side delete-qa-user-data.ts purges while
leaving the Firestore metadata behind.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scanning all 146,105 documents in the collection group confirmed they were the
only two with an RTDB-illegal path segment, and both were deleted on
2026-08-20, so keyNotRtdbSafe now reports zero and the real-data residue is 51
rather than 53.

Worth recording that neither carried a `unit`, so isInClassUnitContainer could
never have misread them — removing them was housekeeping rather than a fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The backfill was scoped under CLUE-610 on the theory that shipping the axes
work without it would break something for users. It would not: the affected
documents never carry `concurrent`, so they cannot reach the container guard
that reads `offeringId`'s absence, and every group document -- the only
population that can reach it -- already has one.

It belongs with the other repairs of the Firestore metadata mirror, which run
their own sweeps rather than waiting on CLUE-604's release gate.

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

no-duplicate-imports treats a type-only import of the same module as a second import,
and a bracket inside a character class needs no escape. Five lint errors, all of which
would have failed the build.

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

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.37%. Comparing base (9a951a5) to head (3cad580).

Files with missing lines Patch % Lines
...s/metadata-repair/backfill-document-offering-id.ts 84.61% 24 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (9a951a5) and HEAD (3cad580). Click for more details.

HEAD has 15 uploads less than BASE
Flag BASE (9a951a5) HEAD (3cad580)
cypress-regression 15 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2980       +/-   ##
===========================================
- Coverage   86.81%   72.37%   -14.44%     
===========================================
  Files        1031     1032        +1     
  Lines       59032    59192      +160     
  Branches    15733    15775       +42     
===========================================
- Hits        51248    42841     -8407     
- Misses       7762    16315     +8553     
- Partials       22       36       +14     
Flag Coverage Δ
cypress-regression ?
cypress-smoke 40.82% <ø> (+0.01%) ⬆️
jest 60.54% <85.71%> (+0.07%) ⬆️

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

cypress Bot commented Aug 25, 2026

Copy link
Copy Markdown

collaborative-learning    Run #20420

Run Properties:  status check passed Passed #20420  •  git commit 3cad5804f3: refactor: move the offeringId backfill into scripts/metadata-repair and share it...
Project collaborative-learning
Branch Review CLUE-643-document-offering-id-backfill
Run status status check passed Passed #20420
Run duration 03m 45s
Commit git commit 3cad5804f3: refactor: move the offeringId backfill into scripts/metadata-repair and share it...
Committer Scott Cytacki
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

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.

🟡 Changes recommended

Input deduplication, RTDB key validation, failure reporting, and stale policy documentation need correction before execution.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a repair workflow to recover missing Firestore offeringId values from Realtime Database metadata.

Changes:

  • Adds a dry-run-first, paginated backfill script with batched writes.
  • Extracts and tests the shared metadata lookup.
  • Documents the repair design, census, and sequencing.
File summaries
File Description
scripts/backfill-document-offering-id.ts Implements the census and repair.
scripts/backfill-document-offering-id.test.ts Tests classification, pagination, and writes.
scripts/lib/document-metadata-lookup.ts Provides the shared RTDB lookup.
scripts/lib/document-metadata-lookup.test.ts Tests lookup outcomes and failures.
scripts/find-documents-missing-metadata.ts Uses the extracted lookup.
docs/superpowers/specs/2026-08-13-clue-643-document-offering-id-backfill-design.md Documents the repair design and findings.
Review details

Suppressed comments (1)

docs/superpowers/specs/2026-08-13-clue-643-document-offering-id-backfill-design.md:402

  • This final section contradicts the resolved decision at lines 303–307: the production census has already established that additional recovery sources are not worthwhile and that unresolved documents should be reported and left alone. Update this section so readers do not treat the policy as pending.
## Open question, deliberately deferred

What to do about documents whose `offeringId` cannot be recovered. Resolving it needs the
production dry run's numbers — specifically the split between `noMetadataNode` and
`nodeWithoutOfferingId`, and how the residue distributes across spaces and types. The options are
building the offering-tree source, building the fuzzy portal-API match, accepting a documented
residue, or deciding the affected documents are dead and should be deleted. Choosing now would be
guessing.
  • Files reviewed: 6/6 changed files
  • Comments generated: 4
  • Review effort level: Balanced

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

Comment on lines +106 to +111
const kRtdbIllegal = /[.#$[\]/]/;

/** Whether every path segment the lookup would build from this document is legal in the RTDB. */
export function isRtdbAddressable(contextId: string, uid: string, key: string): boolean {
return ![contextId, uid, key].some((segment) => kRtdbIllegal.test(segment));
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in "fix: treat a key with an ASCII control character as unaddressable". U+0000–U+001F and U+007F are now rejected alongside .#$[]/, so such a document is counted as keyNotRtdbSafe rather than lookupError. The table-driven test includes \u0000, \n, \u001f and \u007f for each segment.

Comment on lines +217 to +226
export function parseTypes(raw: string | undefined): readonly string[] {
const requested = (raw ?? "").split(",").map((t) => t.trim()).filter(Boolean);
if (requested.length === 0) return kOfferingContainedTypes;
const unknown = requested.filter((t) => !kOfferingContainedTypes.includes(t as any));
if (unknown.length > 0) {
throw new Error(`TYPES names unknown type(s): ${unknown.join(", ")}. ` +
`Known types are: ${kOfferingContainedTypes.join(", ")}`);
}
return requested;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in "fix: scan a type named twice in TYPES only once". parseTypes now deduplicates the list, keeping first-seen order, with a test.

Comment on lines +288 to +295
const report = () => {
log(`scanned ${result.scanned}; ` +
kAllBuckets.map((b) => `${b}: ${result.totals[b]}`).join(", "));
// Said out loud because a Firestore equality query on `type` cannot return a document that has no
// `type` field, so such documents are invisible to this census rather than counted as clean.
log("documents with no `type` field are not reachable by these queries and are not counted");
if (dryRun) log("DRY RUN — set APPLY=1 to write");
};

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in "fix: print the whole partial result when a run fails". On a failed query or commit, the function now logs the complete result: scanned, written, and the per-type and per-space counts. It also attaches that result to the error as err.result before rethrowing. A test fails the second commit and checks that the 400 committed writes and the per-space counts are reported.

Related change: writes now use update() rather than set(..., { merge: true }), so a document deleted between the scan and the commit is not recreated holding only an offeringId. A batch that fails with NOT_FOUND is retried one document at a time. A deleted document is counted as deletedDuringRun and logged instead of ending the run.

scytacki and others added 3 commits September 22, 2026 17:24
…UE-643]

The realtime database also rejects U+0000-U+001F and U+007F in a path.
A key carrying one passed the check, so the lookup threw and the
document was counted as a transient lookupError rather than as
permanently keyNotRtdbSafe.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
TYPES=problem,problem scanned and counted every problem document twice,
and in apply mode could queue the same document again in the second
pass.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
After a failed query or commit, the finally block printed only the
totals, and main()'s final JSON never ran. So an interrupted apply did
not say how many writes had committed, or where. The full result is now
logged on failure and attached to the error.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
scytacki and others added 3 commits September 22, 2026 17:25
The write policy and the closing section still called the policy for
unrecoverable documents an open question, although the production
census settled it: report them and leave them alone.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-643]

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…t stays deleted [CLUE-643]

set with merge writes whether or not the document exists, so a
document deleted between the scan and the commit was recreated holding
only an offeringId. update refuses a missing document on the server.
Because that refusal fails the whole batch, the batch is retried one
document at a time, and a deleted document is counted as
deletedDuringRun and logged instead of ending the run.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
scytacki and others added 2 commits September 22, 2026 18:43
…nd share its helpers [CLUE-643]

The backfill is the last step of the metadata repair, so it now lives
beside the other repair scripts, and their README is its runbook. It
imports isRtdbAddressable and resolveDatabaseUrl from the repair's
shared modules instead of keeping its own copies, and its duplicate
isRtdbAddressable tests are dropped in favor of the ones beside the
shared function.

Co-Authored-By: Claude Opus 5.5 (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.

Comment on lines +20 to +21
It identifies the offering container by the **absence** of `offeringId`. A problem document that
has a `unit` but no `offeringId` therefore reads as class-unit-contained — the wrong container.
Comment on lines +3 to +5
// `isInClassUnitContainer` (src/models/document/document-axes.ts) identifies the offering container by
// the ABSENCE of `offeringId`, so an offering-contained document without one reads as belonging to the
// class's copy of the unit — the wrong container. This script makes the data true so that guard can be

This branch was previously deployed

1 inactive deployment
development 3cad5804 Deployed Sep 22, 2026 by github-actions[bot]
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.

2 participants