CLUE-650: run the Firestore rules tests on every push

[scytacki]

CLUE-650

The firebase-test suite covers every rule in firestore.rules — 408 assertions across 10 files — and no workflow ran it. grep -rln firebase-test .github/workflows/ returns nothing, so it only ever ran when someone remembered to run it locally. That is a weak guarantee for a file that is deployed by hand, often ahead of the app that depends on it.

This adds a firebase-rules job to CI Base and removes the two frictions that kept the suite out of CI.

Why it couldn't just be a step

Its dependencies aren't installed. firebase-test has its own package-lock.json, which the root npm ci never touches. Solved with a working-directory: ./firebase-test install, the same shape the jest job already uses for authoring-api.

test:exec invokes a bare firebase. It worked only on a machine with a global firebase-tools; a runner has none. Rather than npx-ing a version per run, firebase-tools is now a devDependency of firebase-test, so npm ci in that directory is the whole setup — for CI and for a new contributor alike.

The Java part

Pinning firebase-tools pins a Java floor. Version 15 hard-fails rather than starting the emulator below Java 21:

// 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);
}

The runner's default is Java 17, so the job needs to select 21. That costs nothing: ubuntu-latest ships 8, 11, 17, 21 and 25 preinstalled, so setup-java resolves 21 out of the image's toolcache without downloading a JDK.

Deploy gating

The job notifies Slack on a master failure, alongside jest and cypress. It deliberately does not appear in s3-deploy's needs: firestore.rules is deployed on its own, so an emulator hiccup shouldn't hold up an app deploy.

Also

• CLAUDE.md's testing note asked for "Node.js 16.x and Java", both wrong. It now gives the two commands, says why the root install doesn't cover them, and names the real Java floor.
• firebase-test/package.json declared engines: {node: "16"}, which made every install there print an EBADENGINE warning under the Node 20 the rest of the repo uses. It now declares >=20, and the lockfile's root package entry records the same floor.

Verification

Locally, on a node_modules wiped and reinstalled from the lock file, with no global firebase-tools on the path:

$ npm ci --prefix firebase-test && npm --prefix firebase-test run test:exec
Test Suites: 10 passed, 10 total
Tests:       2 skipped, 408 passed, 410 total
✔  Script exited successfully (code 0)

The job itself is verified by this PR's own CI run.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.57%. Comparing base (477a936) to head (b09b6ae).
⚠️ Report is 215 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (477a936) and HEAD (b09b6ae). Click for more details.

HEAD has 20 uploads less than BASE

Flag	BASE (477a936)	HEAD (b09b6ae)
cypress-regression	14	0
cypress	6	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2976       +/-   ##
===========================================
- Coverage   86.13%   70.57%   -15.57%     
===========================================
  Files         988      983        -5     
  Lines       56593    56569       -24     
  Branches    14944    14942        -2     
===========================================
- Hits        48749    39925     -8824     
- Misses       7824    16608     +8784     
- Partials       20       36       +16     

Flag	Coverage Δ
cypress	?
cypress-regression	?
cypress-smoke	41.53% <ø> (-0.03%)	⬇️
jest	57.50% <ø> (ø)

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 #20182

Run Properties:   Passed #20182  •  b09b6ae9ee: ci: sync the firebase-test lockfile with its Node engine floor

Project	collaborative-learning
Branch Review	ci-firebase-rules-tests
Run status	Passed #20182
Run duration	03m 40s
Commit	b09b6ae9ee: ci: sync the firebase-test lockfile with its Node engine floor
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

Adds Firestore security-rule tests to CI on every push.

Changes:

• Adds a Java 21 Firebase rules test job and Slack failure notification.
• Adds local firebase-tools and updates Node requirements.
• Documents test setup and runtime requirements.

Reviewed changes

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

File	Description
.github/workflows/ci.yml	Runs Firestore rules tests in CI.
firebase-test/package.json	Adds Firebase CLI and Node 20 requirement.
CLAUDE.md	Documents Firestore test prerequisites and commands.

---

💡 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 🤖, identifies a few issues to consider before merging.

PR Review Summary

Changes: 4 files, +13066 / -641 lines (13032 of which are firebase-test/package-lock.json)

What it does

The firebase-test suite — 408 assertions across 10 files covering firestore.rules — was never
run by any workflow. Since firestore.rules is deployed by hand, often ahead of the app that
depends on it, the only thing standing between a broken rule and production was someone remembering
to run the suite locally. This adds a firebase-rules job to CI Base so the suite runs on every
push.

The approach

Two frictions kept the suite out of CI, and both are removed rather than worked around:

• Dependencies weren't installed. firebase-test has its own package-lock.json that the root
  npm ci never touches, so the job does a working-directory: ./firebase-test install — the same
  shape the jest job already uses for authoring-api (ci.yml:88-90 vs. ci.yml:49-51).
• test:exec invoked a bare firebase. That only worked on a machine with a global
  firebase-tools. Rather than npx-ing a version per run, firebase-tools@^15.28.1 becomes a
  devDependency of firebase-test, so npm ci in that directory is the entire setup — for CI and
  for a new contributor alike. This is what accounts for the 13k-line lockfile churn.

Pinning firebase-tools 15 pins a Java floor: v15 hard-fails rather than starting the emulator below
Java 21, so the job adds setup-java@v4 with temurin 21. The CI log confirms this costs nothing —
Resolved Java 21.0.12+1 from tool-cache, no download.

The job notifies Slack on a master failure (needs: [jest,cypress,firebase-rules], ci.yml:187) but
deliberately stays out of s3-deploy's needs, on the reasoning that firestore.rules deploys
separately so an emulator hiccup shouldn't hold up an app deploy.

Two loose ends get tidied: CLAUDE.md's testing note (which asked for "Node.js 16.x and Java", both
wrong) now gives the two real commands and the real Java floor, and firebase-test's
engines: {node: "16"} becomes >=20, silencing an EBADENGINE warning on every install there.

Assessment

The approach is right and the execution is clean. Making firebase-tools a local devDependency
rather than an npx incantation is the better call: it makes CI and a fresh clone identical, and
the lockfile makes the version — and therefore the Java floor — explicit rather than a property of
whatever happens to be on someone's PATH.

The claims in the description hold up against the actual CI run
(job 99524181001):
the job passed in 1m35s with Test Suites: 10 passed, Tests: 2 skipped, 408 passed, 410 total,
and exit code 0 propagated through emulators:exec. The stack trace near the end of the log is the
benign --detectOpenHandles TCPWRAP report, not a failure. The lockfile is internally consistent
(root entry records engines: {node: ">=20"} to match package.json, firebase-tools resolves to
15.28.1), the emulator debug logs the run drops in firebase-test/ are already covered by
.gitignore's *debug.log, and there is no dependabot config, so the 1252-package tree in a
test-only directory won't generate PR noise.

The ci.yml comment about the emulator walking up to the repo root for firebase.json is correct and
worth having — it's the non-obvious part of running the job from a subdirectory.

Two things below. Neither is a defect in the diff; the first is a doc the change makes stale, and
the second is a repo setting the diff can't reach.

Issues

1. README.md's database-rules section is now actively misleading — major, high confidence

README.md:59-68 is the human-facing counterpart to the CLAUDE.md note this PR fixed, and it
still says:

• "The tests currently only run with Node.js version 16.x"
• "You need the firebase CLI. Version 12 is compatible with Node 16: npm install -g firebase-tools@12"
• a brew install java recipe with no mention of a version floor

All three are contradicted by this PR. firebase-test/package.json now declares
engines: {node: ">=20"}, and firebase-tools@15.28.1 itself requires node >=20.0.0. A
contributor who follows the README onto Node 16 can't npm ci in that directory at all. The global
firebase-tools@12 install is now both unnecessary (it's a devDependency) and inert (npm scripts
resolve firebase from node_modules/.bin first, so the local 15 wins) — and it's the instruction
most likely to send someone down a wrong path when the emulator then demands Java 21, since that's
the one requirement the README doesn't state.

Worth folding the same three facts into README that went into CLAUDE.md, plus mentioning test:exec
(the README only documents the two-terminal npm run start & + npm run test dance, which CI has
now superseded).

2. The new job cannot block a merge — minor, confirmed

master's branch protection lists exactly three required checks: Build, Run Jest Tests,
cypress (1). Run Firebase Rules Tests is not among them, and by design it isn't in s3-deploy's
needs either. So a PR whose rules tests fail is still green enough to merge, and the first signal
is the Slack ping after it lands on master. That's a real improvement over "nothing ran it," but
it's short of the guarantee the description reaches for. Adding the context to branch protection is
a repo setting outside this diff — flagging it so it doesn't get lost.

3. CLAUDE.md names the Java floor but not how to meet it — nit

The note says firebase-tools 15 needs Java 21 and stops there, which leaves the reader to discover
that this is a one-line JAVA_HOME selection rather than an install — e.g.
JAVA_HOME=$(/usr/libexec/java_home -v21) npm --prefix firebase-test run test:exec on macOS, where
a 21 JDK is usually already present alongside an older default. Same for the runner, where the
setup-java step is doing exactly that selection.