Add multi-layer code coverage pipeline (Codecov, informational v1)#778
Open
jon-bell wants to merge 13 commits into
Open
Add multi-layer code coverage pipeline (Codecov, informational v1)#778jon-bell wants to merge 13 commits into
jon-bell wants to merge 13 commits into
Conversation
…, Postgres)
Wires per-layer coverage into Codecov as 5 informational flags so we can
calibrate thresholds against real PRs before flipping to required checks:
- jest → coverage/jest/lcov.info (built-in Istanbul)
- next-server → NODE_V8_COVERAGE + c8 report
- next-client → Playwright startJSCoverage + v8-to-istanbul (Chromium only)
- edge-functions → custom Deno bootstrap (supabase/functions/_coverage/serve.ts)
that monkey-patches Deno.serve, loads all 47 functions,
and routes them under `deno run --coverage`. Required
because the Supabase edge-runtime fork doesn't support
--coverage and `supabase functions serve` doesn't forward
Deno flags.
- postgres → plpgsql_check profiler + custom lcov bridge that maps
profiler line offsets back to supabase/migrations/*.sql
See COVERAGE.md for v1 limits (DB-trigger-driven edge invocations are not
captured — v2 will reroute Kong) and the local-dev recipe. codecov.yml is
informational-only for v1; flip after ~2 weeks of calibration.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. 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 (2)
WalkthroughAdds multi-layer coverage: CI workflow for Jest and e2e (Next server/client, Edge, Postgres), collection/conversion scripts, Playwright instrumentation, Deno edge bootstrap, Postgres profiler integration, and per-flag Codecov uploads. ChangesEnd-to-End Code Coverage Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 9
🧹 Nitpick comments (10)
scripts/coverage/v8-client-to-lcov.ts (1)
103-111: 💤 Low valueInline comment would clarify the inverted boolean logic.
The
v8-to-istanbulfilter callback returnstrueto exclude andfalseto include, which is the opposite of typical filter semantics. A brief comment would help future readers.📝 Suggestion
(filepath: string) => { - // Filter source files: keep app code, drop node_modules + webpack - // runtime + RSC payloads. + // Filter callback: return `true` to EXCLUDE, `false` to INCLUDE. + // We keep app code, drop node_modules + webpack runtime + RSC payloads. const norm = filepath.replace(/^file:\/\//, "");🤖 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 `@scripts/coverage/v8-client-to-lcov.ts` around lines 103 - 111, Add an inline comment above the v8-to-istanbul filter callback in scripts/coverage/v8-client-to-lcov.ts explaining that the callback returns true to exclude a file and false to include it (i.e., inverted from Array.filter), and briefly note why we exclude paths containing "/node_modules/" or "/webpack/" and include only files under repoRoot (refer to the callback and the local variable norm used for path normalization).scripts/coverage/collect.sh (1)
64-70: 💤 Low valueConsider adding a precondition check for Postgres coverage.
Unlike the other coverage layers which check for their respective directories (
coverage/edge,coverage/server,coverage/client), the Postgres step runs unconditionally. Ifplpgsql_checkprofiler isn't set up viasetup-pg.sh, this will always fail with a warning.For consistency, consider checking for an indicator that postgres coverage was enabled (e.g., a sentinel file or env var), or at minimum documenting that this step is expected to fail gracefully when profiler isn't configured.
🤖 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 `@scripts/coverage/collect.sh` around lines 64 - 70, The Postgres coverage step runs unconditionally and should be gated by a precondition; modify the collect.sh block that runs "npx tsx scripts/coverage/dump-pg.ts" so it only executes when a sentinel or env flag indicates Postgres profiling is enabled (for example check a file like coverage/postgres.enabled or an env var PG_COVERAGE=1), otherwise skip with a clear log message; reference the dump script name (scripts/coverage/dump-pg.ts), output files (coverage/postgres.lcov, coverage/postgres.log) and the setup script/feature (setup-pg.sh / plpgsql_check) so the check aligns with how Postgres coverage is configured.scripts/coverage/dump-pg.ts (1)
84-85: 💤 Low valueConsider validating the dollar-quote tag before regex construction.
While the risk is low (dollar-quote tags in migrations are typically short and safe), constructing a regex from untrusted or variable input can lead to ReDoS attacks if the pattern is malicious.
🛡️ Suggested safeguard
if (m) { dollarTag = m[1]; + // Sanity check: dollar tags should be simple identifiers + if (!/^\w*$/.test(dollarTag)) { + console.warn(`[buildFunctionIndex] Unexpected dollarTag: ${dollarTag} in ${file}:${j+1}`); + continue; + } bodyStart = j + 1; // 1-based, on the line AFTER the AS $tag$Based on static analysis hint flagging RegExp construction from variable input (CWE-1333).
🤖 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 `@scripts/coverage/dump-pg.ts` around lines 84 - 85, The code builds a RegExp from variable dollarTag when creating closeRe, which can be abused; validate or escape dollarTag before constructing the regex. Restrict dollarTag to a safe pattern (e.g., only alphanumerics and underscores using a validation like /^[A-Za-z0-9_]+$/) and throw or fallback if it fails, or escape all regex metacharacters in dollarTag before passing it to new RegExp; update the code path that computes closeRe (the variable closeRe and the source value dollarTag in dump-pg.ts) to perform this validation/escaping prior to RegExp construction.scripts/coverage/setup-pg.sh (1)
39-44: ⚡ Quick winConsider adding an explicit timeout failure message.
If the container doesn't become ready within 30 seconds, the script silently continues and the subsequent
psqlcommand will fail with a less clear error. Adding an explicit check would improve debuggability.♻️ Proposed improvement
# Wait for it to come back. + ready=false for _ in $(seq 1 30); do if docker exec -i "$CONTAINER" pg_isready -U postgres >/dev/null 2>&1; then + ready=true break fi sleep 1 done + if [ "$ready" != "true" ]; then + echo "[setup-pg] ERROR: $CONTAINER did not become ready after 30 seconds" >&2 + exit 1 + fi🤖 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 `@scripts/coverage/setup-pg.sh` around lines 39 - 44, The readiness loop using pg_isready (for _ in $(seq 1 30) ... pg_isready -U postgres) can silently time out; change it to detect failure and exit with a clear error message: after the loop check whether pg_isready ever succeeded (e.g., using a success flag or re-running pg_isready once) and if not call echo to print "Postgres container $CONTAINER did not become ready within 30s" and exit 1 so downstream psql commands fail fast and provide a useful message.tests/e2e/group-create-errors.manual.spec.ts (1)
11-11: ⚡ Quick winConvert this import to the
@/*alias form.This keeps test files aligned with the repository import convention.
Suggested change
-import { expect, test } from "../global-setup"; +import { expect, test } from "`@/tests/global-setup`";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/e2e/group-create-errors.manual.spec.ts` at line 11, Replace the relative import of the test helpers with the project path alias: change the import of the module named "global-setup" (currently imported via "../global-setup") to use the "`@/global-setup`" alias so the test file follows the repository convention and resolves from the project root; update the import statement in group-create-errors.manual.spec.ts to reference "`@/global-setup`" (ensure tsconfig/paths already supports the "@" alias).tests/e2e/csp-smoke.spec.ts (1)
1-1: ⚡ Quick winUse
@/*alias forglobal-setupimport.Switch this relative path to the root alias style used across the repo.
Suggested change
-import { expect, test } from "../global-setup"; +import { expect, test } from "`@/tests/global-setup`";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/e2e/csp-smoke.spec.ts` at line 1, Update the import in tests/e2e/csp-smoke.spec.ts to use the project root path alias instead of a relative path: replace the current import of global-setup (import { expect, test } from "../global-setup") with the aliased form (import { expect, test } from "`@/global-setup`") so the file uses the repository-wide `@/*` alias convention.tests/e2e/instructor-group-management.spec.ts (1)
10-10: ⚡ Quick winUse project-root alias for
global-setupimport.Please switch this to
@/tests/global-setupto comply with the path alias rule.Suggested change
-import { expect, test } from "../global-setup"; +import { expect, test } from "`@/tests/global-setup`";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/e2e/instructor-group-management.spec.ts` at line 10, Update the import in the test file to use the project-root path alias: replace the current import of "../global-setup" with the alias "`@/tests/global-setup`" so the file imports from the project root as required by the path alias rule; ensure the import statement that references global-setup is updated accordingly.tests/e2e/submission-history-review-realtime.spec.ts (1)
22-22: ⚡ Quick winSwitch to
@/*alias for this shared setup import.Use the root alias path to stay consistent with repository standards.
Suggested change
-import { expect, test } from "../global-setup"; +import { expect, test } from "`@/tests/global-setup`";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/e2e/submission-history-review-realtime.spec.ts` at line 22, Replace the relative import of the shared test setup with the project root alias: change the import line "import { expect, test } from \"../global-setup\";" to use the "`@/`..." alias (e.g. "import { expect, test } from \"`@/tests/global-setup`\";") so the shared setup is referenced via the root path alias rather than a relative path.tests/e2e/active-submission-gradebook-db.spec.ts (1)
1-1: ⚡ Quick winUse
@/*alias for shared test setup import.Please replace the relative import with a project-root alias to match repo conventions.
Suggested change
-import { expect, test } from "../global-setup"; +import { expect, test } from "`@/tests/global-setup`";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/e2e/active-submission-gradebook-db.spec.ts` at line 1, Replace the relative import in tests/e2e/active-submission-gradebook-db.spec.ts that reads `import { expect, test } from "../global-setup";` with the project-root alias form using `@/*`, e.g. `import { expect, test } from "`@/tests/global-setup`";` so the file uses the repo's path-alias convention (adjust the alias target if your global-setup lives at a different path under the project root).tests/e2e/finalize-submission-early-group-rpc.spec.ts (1)
1-1: ⚡ Quick winUse root alias import instead of relative path.
Please use
@/tests/global-setuphere for consistency with the TS/JS alias rule.Suggested change
-import { expect, test } from "../global-setup"; +import { expect, test } from "`@/tests/global-setup`";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/e2e/finalize-submission-early-group-rpc.spec.ts` at line 1, Replace the relative import in the test file that currently reads import { expect, test } from "../global-setup"; with the root-alias form import { expect, test } from "`@/tests/global-setup`"; to comply with the project's TS/JS alias rule; update only the module specifier (the import source string) so other code remains unchanged.
🤖 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 @.github/workflows/coverage.yml:
- Around line 27-29: The checkout step using actions/checkout@v4 leaves the
GITHUB_TOKEN in the local git config; update the checkout invocation(s) (the
steps that use actions/checkout@v4 around the existing fetch-depth: 0 entries)
to include persist-credentials: false so credentials are not persisted to the
repo config during later artifact/log handling (apply the same change to the
other checkout block referenced in the comment).
- Around line 150-153: The readiness-wait loops using "for _ in $(seq 1 60); do
... curl -fsS http://localhost:3001/ ...; done" currently continue silently on
timeout; change them to fail-fast by checking after the loop (or appending an
explicit exit) and exit with a non-zero status when curl never succeeds (e.g.,
detect that the loop completed without breaking and call exit 1), and apply the
same change to the second loop at the other occurrence so the workflow stops
immediately with a clear error instead of letting Playwright run against an
unavailable service.
- Around line 3-5: The workflow-level permissions currently include
"pull-requests: write" which is overly broad; remove "pull-requests: write" from
the top-level permissions block and instead grant "pull-requests: write" only
inside the specific job that needs it (e.g., the codecov/upload or comment job)
by adding a permissions subsection in that job with pull-requests: write while
keeping the root permissions to minimal read-only scopes like contents: read.
- Line 27: Replace mutable tag refs in the workflow with immutable commit SHAs:
locate the `uses:` entries `actions/checkout@v4`, `actions/setup-node@v4`,
`codecov/codecov-action@v5`, and `denoland/setup-deno@v2` in
`.github/workflows/coverage.yml` and pin each to the corresponding full commit
SHA (e.g., `actions/checkout@<commit-sha>`), updating each `uses:` line so the
actions are referenced by their exact commit SHAs rather than tag-only refs to
strengthen supply-chain guarantees.
In `@scripts/coverage/init-pg.sql`:
- Around line 16-20: The comment in init-pg.sql incorrectly refers to
"init-pg-session.sql below" which isn't in this PR; update the comment to
reflect how profiler is actually enabled (via the global ALTER SYSTEM SET
plpgsql_check.profiler = on in setup-pg.sh) or remove the session-file reference
entirely. Specifically, edit the explanatory block (the lines mentioning
"init-pg-session.sql below" and session-level profiler enablement) to either
point to the global setup mechanism (mentioning ALTER SYSTEM SET
plpgsql_check.profiler = on) or delete the misleading sentence so the comment
only describes the shared/global reset behavior performed by init-pg.sql.
In `@scripts/coverage/run-edge-bootstrap.sh`:
- Line 24: The deno bootstrap command is granting --allow-ffi unnecessarily;
edit the deno run invocation that launches the coverage serve (the `deno run`
command invoking the coverage serve.ts) and remove the `--allow-ffi` flag from
its permissions list so the script no longer requests FFI access.
In `@supabase/functions/_coverage/serve.ts`:
- Around line 130-133: The catch block that builds the error Response currently
returns internal error details via String(err) (the Response constructed in
serve.ts); instead, log the actual error to server logs (e.g., using
console.error or the existing logger) and replace the Response body with a
generic client-safe message such as { error: "Internal server error" } and keep
the 500 status and content-type header; locate the Response construction where
new Response(JSON.stringify({ error: String(err) }), { status: 500, headers: {
"content-type": "application/json" } }) is used and update it to log the err and
return the generic payload.
- Around line 96-100: The catch block that currently logs import failures
(`console.error(`[coverage-bootstrap] failed to load ${name}:`, err)`) must
record failures and cause a non-zero exit after discovery; add a collection
(e.g., `failedModules`) and push `name` (and optionally `err`) inside the catch,
continue to attempt other imports, and after the module-loading loop check
`failedModules.length > 0` and call `processLogger.error`/`console.error` with a
summary of failed modules and then `process.exit(1)` so CI fails instead of
producing partial coverage.
In `@utils/supabase/server.ts`:
- Around line 33-40: The try/catch around setting FUNCTIONS_URL_OVERRIDE in
utils/supabase/server.ts currently only console.warns on failure; change it to
fail fast during coverage by throwing the error instead of swallowing it when
coverage is active (e.g. if process.env.COVERAGE or globalThis.__coverage__ is
truthy). Update the catch in the block that touches FUNCTIONS_URL_OVERRIDE,
client.functionsUrl and (client.functions as ...).url to rethrow a new Error
(including the original err) when coverage is detected, otherwise preserve the
existing console.warn behavior.
---
Nitpick comments:
In `@scripts/coverage/collect.sh`:
- Around line 64-70: The Postgres coverage step runs unconditionally and should
be gated by a precondition; modify the collect.sh block that runs "npx tsx
scripts/coverage/dump-pg.ts" so it only executes when a sentinel or env flag
indicates Postgres profiling is enabled (for example check a file like
coverage/postgres.enabled or an env var PG_COVERAGE=1), otherwise skip with a
clear log message; reference the dump script name (scripts/coverage/dump-pg.ts),
output files (coverage/postgres.lcov, coverage/postgres.log) and the setup
script/feature (setup-pg.sh / plpgsql_check) so the check aligns with how
Postgres coverage is configured.
In `@scripts/coverage/dump-pg.ts`:
- Around line 84-85: The code builds a RegExp from variable dollarTag when
creating closeRe, which can be abused; validate or escape dollarTag before
constructing the regex. Restrict dollarTag to a safe pattern (e.g., only
alphanumerics and underscores using a validation like /^[A-Za-z0-9_]+$/) and
throw or fallback if it fails, or escape all regex metacharacters in dollarTag
before passing it to new RegExp; update the code path that computes closeRe (the
variable closeRe and the source value dollarTag in dump-pg.ts) to perform this
validation/escaping prior to RegExp construction.
In `@scripts/coverage/setup-pg.sh`:
- Around line 39-44: The readiness loop using pg_isready (for _ in $(seq 1 30)
... pg_isready -U postgres) can silently time out; change it to detect failure
and exit with a clear error message: after the loop check whether pg_isready
ever succeeded (e.g., using a success flag or re-running pg_isready once) and if
not call echo to print "Postgres container $CONTAINER did not become ready
within 30s" and exit 1 so downstream psql commands fail fast and provide a
useful message.
In `@scripts/coverage/v8-client-to-lcov.ts`:
- Around line 103-111: Add an inline comment above the v8-to-istanbul filter
callback in scripts/coverage/v8-client-to-lcov.ts explaining that the callback
returns true to exclude a file and false to include it (i.e., inverted from
Array.filter), and briefly note why we exclude paths containing "/node_modules/"
or "/webpack/" and include only files under repoRoot (refer to the callback and
the local variable norm used for path normalization).
In `@tests/e2e/active-submission-gradebook-db.spec.ts`:
- Line 1: Replace the relative import in
tests/e2e/active-submission-gradebook-db.spec.ts that reads `import { expect,
test } from "../global-setup";` with the project-root alias form using `@/*`,
e.g. `import { expect, test } from "`@/tests/global-setup`";` so the file uses the
repo's path-alias convention (adjust the alias target if your global-setup lives
at a different path under the project root).
In `@tests/e2e/csp-smoke.spec.ts`:
- Line 1: Update the import in tests/e2e/csp-smoke.spec.ts to use the project
root path alias instead of a relative path: replace the current import of
global-setup (import { expect, test } from "../global-setup") with the aliased
form (import { expect, test } from "`@/global-setup`") so the file uses the
repository-wide `@/*` alias convention.
In `@tests/e2e/finalize-submission-early-group-rpc.spec.ts`:
- Line 1: Replace the relative import in the test file that currently reads
import { expect, test } from "../global-setup"; with the root-alias form import
{ expect, test } from "`@/tests/global-setup`"; to comply with the project's TS/JS
alias rule; update only the module specifier (the import source string) so other
code remains unchanged.
In `@tests/e2e/group-create-errors.manual.spec.ts`:
- Line 11: Replace the relative import of the test helpers with the project path
alias: change the import of the module named "global-setup" (currently imported
via "../global-setup") to use the "`@/global-setup`" alias so the test file
follows the repository convention and resolves from the project root; update the
import statement in group-create-errors.manual.spec.ts to reference
"`@/global-setup`" (ensure tsconfig/paths already supports the "@" alias).
In `@tests/e2e/instructor-group-management.spec.ts`:
- Line 10: Update the import in the test file to use the project-root path
alias: replace the current import of "../global-setup" with the alias
"`@/tests/global-setup`" so the file imports from the project root as required by
the path alias rule; ensure the import statement that references global-setup is
updated accordingly.
In `@tests/e2e/submission-history-review-realtime.spec.ts`:
- Line 22: Replace the relative import of the shared test setup with the project
root alias: change the import line "import { expect, test } from
\"../global-setup\";" to use the "`@/`..." alias (e.g. "import { expect, test }
from \"`@/tests/global-setup`\";") so the shared setup is referenced via the root
path alias rather than a relative path.
🪄 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: b44e8f5a-bf55-47ec-b5cb-c2506d5ef726
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsupabase/functions/_coverage/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/coverage.ymlCOVERAGE.mdapp/api/__coverage__/route.tscodecov.ymljest.config.jspackage.jsonscripts/coverage/collect.shscripts/coverage/dump-pg.tsscripts/coverage/init-pg.sqlscripts/coverage/run-edge-bootstrap.shscripts/coverage/setup-pg.shscripts/coverage/v8-client-to-lcov.tssupabase/functions/_coverage/README.mdsupabase/functions/_coverage/deno.jsonsupabase/functions/_coverage/serve.tstests/e2e/active-submission-gradebook-db.spec.tstests/e2e/csp-smoke.spec.tstests/e2e/finalize-submission-early-group-rpc.spec.tstests/e2e/group-create-errors.manual.spec.tstests/e2e/instructor-group-management.spec.tstests/e2e/submission-history-review-realtime.spec.tstests/global-setup.tsutils/supabase/client.tsutils/supabase/server.ts
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
CI fixes:
- setup-pg.sh: plpgsql_check installs into `public` (not its own
schema) in Supabase images, and `postgres` is not a superuser so
ALTER SYSTEM is denied. Detect this and write plpgsql_check.profiler
directly to postgresql.auto.conf via docker exec, restart the
container, and call plpgsql_profiler_reset_all() unqualified via the
default search_path. Add fail-fast on readiness timeout and write a
coverage/.pg-ready sentinel for collect.sh to gate on.
- dump-pg.ts: drop the (wrong) plpgsql_check schema qualifier; rely
on the default search_path. Validate dollar-quote tags before regex
construction to avoid ReDoS (CWE-1333).
- coverage workflow Jest job: provide dummy SUPABASE_URL/ANON_KEY/
SERVICE_ROLE_KEY env so tests/e2e/TestingUtils.ts's
createAdminClient() doesn't blow up at module load. Skip the
pre-existing-broken tests/unit/llm-hint.test.ts (`Request is not
defined` jsdom issue per AGENTS.md) and continue-on-error so the
lcov still uploads.
- coverage workflow .env.local: write GITHUB_PRIVATE_KEY_STRING as a
multi-line quoted PEM instead of \n-escaped single-line; Deno's
--env-file and Next's dotenv both accept the multi-line form, and
the single-line escaped form was passed to octokit literally and
rejected as malformed.
PR review feedback:
- Move pull-requests:write from workflow root to per-job permissions.
- Add persist-credentials:false to both checkout steps.
- Make Next + edge bootstrap startup probes fail-fast instead of
silently timing out.
- Edge bootstrap: fail non-zero when any function module fails to
import (env ALLOW_PARTIAL_LOAD=1 to keep local-dev behavior).
- Edge bootstrap: return generic 500 body instead of String(err) to
callers (CodeQL information-exposure flag).
- utils/supabase/{client,server}.ts: throw in coverage mode when the
functions-URL override can't be applied — silent fallback would
produce zero edge coverage that looked like a test problem.
- collect.sh: gate Postgres dump on coverage/.pg-ready sentinel.
- run-edge-bootstrap.sh: drop --allow-ffi (no edge function uses FFI).
- init-pg.sql: drop stale "init-pg-session.sql" reference; call
plpgsql_profiler_reset_all() unqualified.
- v8-client-to-lcov.ts: clarify inverted filter callback semantics.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/coverage.yml (1)
65-68: 💤 Low value
continue-on-error: truemasks genuine Jest failures.The Jest step will show green even when tests fail (beyond the intentionally skipped one). For v1 informational mode this may be acceptable, but it could hide regressions. Consider removing
continue-on-erroronce the known jsdom issue is resolved, or at least when coverage becomes required.🤖 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 @.github/workflows/coverage.yml around lines 65 - 68, The CI step running "npx jest --coverage --testPathIgnorePatterns='tests/unit/llm-hint\.test\.ts'" currently uses the YAML key continue-on-error: true which masks real test failures; remove or disable continue-on-error for that job (or gate it behind an explicit conditional such as a workflow input or env flag) so the Jest step fails the workflow on test failures, and ensure the step name/step block that runs the Jest command is updated accordingly to no longer include continue-on-error: true until the jsdom issue is resolved.
🤖 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 `@scripts/coverage/setup-pg.sh`:
- Around line 51-60: The success branch after running psql -c "ALTER SYSTEM SET
plpgsql_check.profiler = on;" currently does not reload Postgres, so the new
setting may not take effect; update the success path (the if ! docker exec -i
"$CONTAINER" psql ... check) to invoke a configuration reload via psql (e.g.
docker exec -i "$CONTAINER" psql -U postgres -d postgres -c "SELECT
pg_reload_conf();" or perform a restart) and ensure you set needs_restart
appropriately (keep the existing behavior that sets needs_restart=true only when
the append path was used, or set it to true if you choose restart) so the
profiler setting is applied immediately.
---
Nitpick comments:
In @.github/workflows/coverage.yml:
- Around line 65-68: The CI step running "npx jest --coverage
--testPathIgnorePatterns='tests/unit/llm-hint\.test\.ts'" currently uses the
YAML key continue-on-error: true which masks real test failures; remove or
disable continue-on-error for that job (or gate it behind an explicit
conditional such as a workflow input or env flag) so the Jest step fails the
workflow on test failures, and ensure the step name/step block that runs the
Jest command is updated accordingly to no longer include continue-on-error: true
until the jsdom issue is resolved.
🪄 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: d128cfe7-b0ec-4f59-98bd-5a35266e7183
📒 Files selected for processing (10)
.github/workflows/coverage.ymlscripts/coverage/collect.shscripts/coverage/dump-pg.tsscripts/coverage/init-pg.sqlscripts/coverage/run-edge-bootstrap.shscripts/coverage/setup-pg.shscripts/coverage/v8-client-to-lcov.tssupabase/functions/_coverage/serve.tsutils/supabase/client.tsutils/supabase/server.ts
Comment on lines
+51
to
+60
| if ! docker exec -i "$CONTAINER" psql -U postgres -d postgres \ | ||
| -c "ALTER SYSTEM SET plpgsql_check.profiler = on;" >/dev/null 2>&1; then | ||
| echo "[setup-pg] ALTER SYSTEM denied — appending to postgresql.auto.conf directly" | ||
| # Remove any pre-existing entry to keep idempotency clean, then append. | ||
| docker exec -i "$CONTAINER" bash -c " | ||
| sed -i '/^plpgsql_check\\.profiler/d' /var/lib/postgresql/data/postgresql.auto.conf 2>/dev/null || true | ||
| echo \"plpgsql_check.profiler = 'on'\" >> /var/lib/postgresql/data/postgresql.auto.conf | ||
| " | ||
| needs_restart=true | ||
| fi |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Verify whether setup-pg applies config after successful ALTER SYSTEM.
fd -a "setup-pg.sh" scripts --exec sh -c '
file="$1"
echo "== $file =="
rg -n "ALTER SYSTEM SET plpgsql_check\\.profiler|pg_reload_conf|needs_reload|needs_restart" "$file"
' sh {}Repository: pawtograder/platform
Length of output: 351
Post-ALTER SYSTEM success path never reloads/restarts plpgsql_check.profiler
In scripts/coverage/setup-pg.sh (around lines 51-79), plpgsql_check.profiler is set via ALTER SYSTEM SET ..., but the success path only leaves needs_restart as false and there’s no pg_reload_conf()/reload logic afterward—so the setting may not take effect until a reload/restart happens.
Add a pg_reload_conf() (or restart) on the successful ALTER SYSTEM branch, matching the behavior you already trigger on the failure/append path.
🤖 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 `@scripts/coverage/setup-pg.sh` around lines 51 - 60, The success branch after
running psql -c "ALTER SYSTEM SET plpgsql_check.profiler = on;" currently does
not reload Postgres, so the new setting may not take effect; update the success
path (the if ! docker exec -i "$CONTAINER" psql ... check) to invoke a
configuration reload via psql (e.g. docker exec -i "$CONTAINER" psql -U postgres
-d postgres -c "SELECT pg_reload_conf();" or perform a restart) and ensure you
set needs_restart appropriately (keep the existing behavior that sets
needs_restart=true only when the append path was used, or set it to true if you
choose restart) so the profiler setting is applied immediately.
pg_isready only confirms Postgres is accepting connections. Supabase's auth, kong, realtime, and storage all depend on the DB and take a few seconds to reconnect after the bounce. The next workflow step (\`npx supabase status -o env\`) fails fast if any of those still report as starting, which crashed the E2E coverage job at the .env.local write step. Poll \`npx supabase status\` until it returns 0 (or warn after 120s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
\`npx supabase status -o env\` emits \`API_URL=\`, \`ANON_KEY=\`, \`SERVICE_ROLE_KEY=\`, \`DB_URL=\` — NOT \`SUPABASE_URL=\` or \`NEXT_PUBLIC_*=\`. We were piping its raw stdout into .env.local, so Next.js + our utils/supabase/client.ts code couldn't find the env it expected and crashed every request with \"SUPABASE_URL or NEXT_PUBLIC_SUPABASE_URL is required\". Split into two steps: - Resolve: parses \`supabase status -o env\` and writes the prefixed names to \$GITHUB_ENV so all subsequent steps see them in process.env - Write .env.local: writes the same names into .env.local for Next's dotenv loader and the edge bootstrap's --env-file Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deno 2.x's default import allowlist only includes deno.land, jsr.io, esm.sh, cdn.jsdelivr.net, raw.githubusercontent.com, and gist.githubusercontent.com. Three of our edge functions transitively import from denopkg.com (via deno.land/x/sha1's deps), which the default allowlist rejects with: TypeError: Requires import access to "denopkg.com:443", run again with the --allow-import flag This caused 3 of 47 modules to fail to load, which (with the new fail-fast behavior from the last review pass) aborted the bootstrap. Pass bare --allow-import to mirror the production edge-runtime, which does not impose this restriction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deno 2.x exposes Deno.serve as a getter-only accessor on the Deno namespace object, so the bare \`(Deno as any).serve = …\` assignment fails with: TypeError: Cannot set property serve of #<Object> which has only a getter Use Object.defineProperty with writable + configurable to redefine the property in place. Verified locally: 46/47 functions register in ~3s (the missing one is GITHUB_PRIVATE_KEY_STRING-dependent and is generated in CI). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Playwright was running both chromium and webkit projects, but the workflow only installs \`--with-deps chromium\` (we don't need webkit for V8 client coverage). 149 webkit tests marked \"did not run\" and the suite exited 1. Pass --project=chromium to skip it cleanly. V8 coverage flush wasn't happening because the kill was hitting npm's PID, not deno's / next's children (npm doesn't always forward signals). Start servers under \`setsid\` (own process group), then SIGINT the group on shutdown so deno can flush coverage/edge/* and Next can flush NODE_V8_COVERAGE. Belt-and-suspenders pkill afterward for stragglers. Known issue still pending: v8-client-to-lcov's source-map resolution returns files_with_coverage=0 because webpack:// paths don't match repoRoot. Tracked as v2 work; client.lcov uploads as empty for now (Codecov ignores it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deno's --coverage=DIR dumps profiler data on clean process exit. When a Deno HTTP server is running, the default signal handling does NOT trigger a clean exit (the runtime keeps the event loop alive on SIGINT/SIGTERM). Register explicit Deno.addSignalListener calls that invoke Deno.exit(0) so V8 flushes its profile JSON before we tear down. Verified locally: SIGTERM produces ~hundreds of cov_*.json files in the coverage dir. Also: continue-on-error on the Run Playwright step so pre-existing E2E flakes (~13 timeout failures from the same patterns the team fixed in commit 4d0b58e) don't block coverage uploads. The failures remain visible in the step output. This whole workflow is v1 informational; the real E2E gate runs in a separate workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The old v8-to-istanbul implementation produced files_with_coverage=0
because:
1. The build wasn't emitting client sourcemaps. Next only emits .js.map
files in production builds when productionBrowserSourceMaps is true;
we never set it.
2. v8-to-istanbul defaults to fetching .map files via HTTP from the
entry URL, but Next is shut down by the time collect.sh runs.
3. The filter rejected all webpack:// source paths because they don't
start with the absolute repoRoot.
Two changes:
- next.config.ts: set productionBrowserSourceMaps = (COVERAGE === "1")
so the coverage build pipeline gets .js.map files alongside every
chunk in .next/static/chunks/.
- scripts/coverage/v8-client-to-lcov.ts: replace v8-to-istanbul with
monocart-coverage-reports (purpose-built for Playwright + Next).
We pre-attach the on-disk sourcemap to each V8 entry (URL-to-disk
translation: /_next/static/chunks/X.js → .next/static/chunks/X.js.map)
so monocart doesn't need HTTP fetch. Then a sourcePath callback
strips the `_N_E/` webpack package prefix to land paths at
`app/foo.tsx`, `components/X.tsx`, etc. — matching the file paths
Codecov sees from the repo. Callback is idempotent (monocart calls
it recursively) — verified locally with a synthetic 100%-executed
dump producing SF:app/global-error.tsx with the right line counts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Monocart's \`clean\` option defaults to **true** and wipes the entire outputDir before generating reports. Our outputDir is \`coverage/\`, which by the time monocart runs already contains: - edge.lcov (300KB, written by \`deno coverage\` step earlier) - server.lcov (5KB, written by \`c8 report\` step earlier) - .pg-ready (sentinel for the postgres dump step) Last run silently deleted all three; only client.lcov remained. The Codecov uploads for next-server and edge-functions still happened but with no data, and dump-pg skipped because .pg-ready was gone. Set clean: false so monocart writes only its own report file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
URL.pathname keeps percent-encoded characters as-is, so a Chromium request for \`/_next/static/chunks/app/course/%5Bcourse_id%5D/...\` produced a pathname containing literal \`%5B\` rather than \`[\`. The filesystem chunk lives at \`.next/static/chunks/app/course/[course_id]/\` with real brackets, so loadSourceMap() silently failed for every dynamic-route page chunk. That explained why the previous lcov contained only 12 \`SF:app/...\` files despite hundreds of pages being exercised by Playwright — literally every \`[bracket]\` route was failing source-map resolution. Fix: decodeURIComponent on the pathname before joining .next/. Verified locally with the assignments/[assignment_id]/page chunk: both percent-encoded and decoded URL forms now resolve to the same on-disk \`.js.map\`, producing \`SF:app/course/[course_id]/assignments/[assignment_id]/page.tsx\` plus 19 sibling component sources. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/coverage/run-edge-bootstrap.sh (1)
23-29: 💤 Low valueConsider syncing the README with this command.
The
--allow-importflag added here is necessary for Deno 2.x dynamic imports, but the example command insupabase/functions/_coverage/README.md(lines 28-38) doesn't include it. Might be worth a quick update so future readers don't get confused when copy-pasting.🤖 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 `@scripts/coverage/run-edge-bootstrap.sh` around lines 23 - 29, The README example command in supabase/functions/_coverage/README.md must be updated to match the actual script: add the --allow-import flag to the deno run invocation so it mirrors the invocation used in run-edge-bootstrap.sh; locate the example deno run command in the README.md and insert --allow-import into its flag list to keep both commands consistent.
🤖 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.
Nitpick comments:
In `@scripts/coverage/run-edge-bootstrap.sh`:
- Around line 23-29: The README example command in
supabase/functions/_coverage/README.md must be updated to match the actual
script: add the --allow-import flag to the deno run invocation so it mirrors the
invocation used in run-edge-bootstrap.sh; locate the example deno run command in
the README.md and insert --allow-import into its flag list to keep both commands
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 775ae76b-1a8c-4850-9696-b7ad4e39114e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/coverage.ymlnext.config.tspackage.jsonscripts/coverage/run-edge-bootstrap.shscripts/coverage/setup-pg.shscripts/coverage/v8-client-to-lcov.tssupabase/functions/_coverage/serve.ts
Server Components like \`app/(auth-pages)/sign-in/page.tsx\` execute on the Node server, so their coverage lands in NODE_V8_COVERAGE (not in Playwright's V8 dump). But Next 15 omits sourcemaps from server bundles in production by default, so \`c8 report\` had no way to map \`.next/server/app/(auth-pages)/sign-in/page.js\` back to \`.tsx\` — explaining why server.lcov was only 5KB with 4 utility files instead of ~200 page sources. Force \`devtool = "source-map"\` for the server build when COVERAGE=1. Apply only to isServer (client gets sourcemaps via \`productionBrowserSourceMaps: true\` above; setting both produces an empty \`.next/static/\`). Verified locally: \`.next/server\` now contains 320 .js.map files including \`(auth-pages)/sign-in/page.js.map\` whose \`sources\` array points at \`webpack://@pawtograder/webapp/./app/(auth-pages)/sign-in/page.tsx\`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous c8 invocation passed --exclude='.next/**' alongside --include='app/**' etc. By default c8 applies these globs to the dist files (\`.next/server/app/.../page.js\`) BEFORE source-map resolution. Result: every Server Component page bundle matched the .next exclude and got dropped, even though the source map could have remapped it to \`app/.../page.tsx\`. Only the handful of utility files Node loaded outside the bundle (lib/courseFeatures.ts, utils/utils.ts, …) survived. Pass --exclude-after-remap so the globs apply to the resolved source paths instead. Also add components/ to the include list, mirroring the client config. 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.
Summary
Wires per-layer code coverage into Codecov as 5 informational flags so we can calibrate thresholds against real PRs before flipping any required checks. v1 deliberately doesn't block merges.
jestnext-serverNODE_V8_COVERAGE+c8 reportnext-clientstartJSCoverage+v8-to-istanbul(Chromium only)edge-functionsdeno run --coveragepostgresplpgsql_checkprofiler + lcov bridgeWhy an edge-function bootstrap
Supabase's
edge-runtime(forked Deno) doesn't implement V8 coverage andsupabase functions servedoesn't forward Deno flags.supabase/functions/_coverage/serve.tsreplaces it during coverage runs: monkey-patchesDeno.servebefore importing function modules, dynamically imports all 47 functions, and routes by URL path. Validated locally — boots in ~3s, loads functions cleanly when env is present.v1 limitations (see COVERAGE.md)
pg_netcalls from Postgres) bypass the bootstrap and still hit the real edge-runtime container. v2 will reroute Kong.COVERAGE_DEBUG=1to see them).plpgsql_checkrequiresshared_preload_libraries—npm run coverage:setup-pghandles that (re-run after any freshsupabase start).Rollout
informational: truein codecov.yml means none of this blocks a merge yet.informational: falsefor project + patch defaults, add Codecov check to branch protection onstagingas a required status.Files of note
supabase/functions/_coverage/serve.ts— the bootstrap. Standalone Deno; replacessupabase functions servefor coverage runs.scripts/coverage/setup-pg.sh— one-time-per-fresh-DB script that addsplpgsql_checktoshared_preload_librariesand restarts the Postgres container.scripts/coverage/dump-pg.ts— parsessupabase/migrations/*.sqlto build a function→file:line index, queries the profiler, emits lcov mapped back to the migration where each function was last defined..github/workflows/coverage.yml— two parallel jobs (jest fast; e2e long); uses${{ secrets.CODECOV_TOKEN }}(already configured at repo level).codecov.yml— 5 flags, 80% patch / 0.5% project drop, allinformational: truefor v1.tests/global-setup.ts— new_autoCoverageauto-fixture; no-op unlessCOVERAGE=1.@playwright/test→../global-setupso they pick up the auto-fixture.Test plan
jestflag (the e2e job is large; even partial success on a first run is fine — informational only).lint.ymlor other workflows.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
Documentation