[pull] main from Open-Legal-Products:main
From the PR description
See Commits and Changes for more details.
Created by pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )
Our analysis
Merge a nine-commit update — read the full analysis →
Think the analysis missed something the PR description covers?
Commits in this PR (9)
| SHA | Subject | Author | Date | |
|---|---|---|---|---|
44ed40db | ci: run the Supabase RLS/stack integration suite on every PR | Amalanand Muthukumaran | 2026-07-25 | ↗ GitHub |
commit bodyThe gated stack tests (backend/src/__tests__/integration/*.supabase.test.ts) prove the deny-all RLS firewall and the auth<->API contract against a real local Supabase stack, but no CI trigger ever set the SUPABASE_TEST_* env vars, so they silently self-skipped on every PR. Add a workflow that boots the stack on the runner (supabase CLI pinned, minimal service set: db, auth, rest, kong), bootstraps the database the way backend/scripts/test-stack.sh does, and runs the suite. No secrets: everything is local to the runner. The bootstrap loads schema.sql and then applies every dated migration on top in filename order, which doubles as a schema-drift smoke test: the snapshot and the migrations must apply cleanly together. Running it surfaced five migrations that could not apply on top of the current schema.sql - three whose backfills read documents columns that later migrations moved to document_versions, and two overview RPCs whose return row type `create or replace` cannot change. Guard the backfills on the historical columns' existence and drop-before-create the RPCs (the pattern 20260703_02 already uses); behavior on era deployments is unchanged, and the full sequence now applies cleanly end to end (verified locally: fresh stack, schema + 44 migrations, 5/5 stack tests green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> | ||||
22542d8d | ci: bootstrap the stack from schema.sql only; restore historical migrations | Amal | 2026-08-05 | ↗ GitHub |
commit bodyAddresses willchen96's review on PR #256.
WHY THIS MATTERS
The previous revision of this workflow built the CI database as:
empty database -> current schema.sql -> every dated migration
and called it a "schema-drift smoke test". But that sequence is not a real
installation path. Per README.md, this repo has two deliberately separate
database artifacts:
- backend/schema.sql -> the COMPLETE shape for FRESH databases
- backend/migrations/ -> incremental steps that move OLDER, already-
deployed databases forward from the version
they are on
A fresh install runs schema.sql and stops. An existing deployment runs only
the migrations dated after its version. Nobody ever replays the full
migration history on top of the current snapshot - so when five old
migrations "failed" under that replay, they were not broken; the harness
was. They had run correctly on the era-appropriate schemas they were
written for.
WHAT IS MIGRATION IMMUTABILITY
Once a migration has shipped and real deployments have executed it, the
file becomes a historical record of "the change that was required at that
point in time". Editing it afterwards cannot help any database that already
ran it - it only makes the repo's history diverge from what production
actually executed, which is exactly the record you need intact when
debugging a deployment later. (Tools like Flyway enforce this with
checksums: a modified applied migration is a hard error.) The previous
revision rewrote five historical migrations to satisfy the artificial
replay; worse, the column-existence guards it added made those backfills
silently no-op on unexpected schemas - converting the loud failure a drift
check exists to produce into a silent skip.
HOW THIS COMMIT FIXES IT
1. The five historical migrations are restored byte-for-byte to their
state on main (20260424_01, 20260427_01, 20260602_01, 20260613_02,
20260613_05).
2. The workflow's bootstrap step now loads schema.sql only - the same
thing backend/scripts/test-stack.sh does locally and the same thing the
README documents for a fresh deployment. The suite therefore tests the
real contract: "a fresh Mike database enforces deny-all RLS and the
auth<->API contract."
Real drift protection (does baseline-plus-migrations equal the current
snapshot?) needs a pinned baseline dump from an older release, the
migrations dated after it, and a pg_dump schema diff against a
schema.sql-built database. That is a separate change, designed separately.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| ||||
78a0a40c | ci: run tabularPagination.supabase.test.ts in the stack workflow | Amal | 2026-08-05 | ↗ GitHub |
commit bodyWHY THIS MATTERS
The whole point of this PR is to close a silent-skip gap: the Supabase
stack suites gate themselves on SUPABASE_TEST_* env vars and quietly
self-skip when those are absent, so plain CI never actually exercised
them. The new stack-tests workflow boots a real Supabase stack and runs
the suites with the env vars set - but its vitest invocation listed only
two of the three files that backend/scripts/test-stack.sh (the declared
source of truth) runs:
# test-stack.sh runs:
stack.supabase.test.ts
access.supabase.test.ts
tabularPagination.supabase.test.ts # <-- missing from CI
So tabularPagination.supabase.test.ts kept doing in CI exactly what
this PR exists to stop: skipping silently. Worse, the workflow comment
("The exact suite test-stack.sh invokes") claimed parity that did not
exist, which is how this kind of drift survives review.
WHAT IS A SELF-SKIPPING (GATED) SUITE
A gated suite decides at load time whether to run, based on the
environment it finds:
const url = process.env.SUPABASE_TEST_URL;
const serviceKey = process.env.SUPABASE_TEST_SERVICE_ROLE_KEY;
const maybeDescribe = url && serviceKey ? describe : describe.skip;
This is great for local ergonomics (checkout works without Docker), but
dangerous in CI: a skipped suite exits 0 and looks green. The only
defense is a workflow that provably sets the gate variables AND lists
every gated file - a list that must be kept in lockstep with the local
runner script, or files fall through the crack unnoticed.
HOW THE FIX WORKS
Add the missing file to the workflow's vitest invocation so the CI list
matches test-stack.sh file-for-file, making the "exact suite" comment
true. Nothing else needs enabling, verified on this branch:
- Gate variables: the suite gates only on SUPABASE_TEST_URL and
SUPABASE_TEST_SERVICE_ROLE_KEY, both already exported to the job env
by the "Export stack connection env" step.
- Schema: the suite touches only public.projects and
public.tabular_reviews, both created by backend/schema.sql, which the
"Load schema" step applies before tests run.
Verified: workflow YAML parses cleanly, and the exact three-file vitest
invocation loads all suites without error (they self-skip locally where
no stack is running, exit 0 - in CI the exported env vars flip them on).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| ||||
cf4821ad | test(e2e): axe accessibility spec, failure traces, nightly schedule | Amalanand Muthukumaran | 2026-07-25 | ↗ GitHub |
commit body- e2e/accessibility.spec.ts: axe-core scans (@axe-core/playwright) over /login (pre-auth), /assistant, /projects, and /tabular-reviews. Two-tier policy: critical-impact violations fail the build; serious-impact violations are logged without failing, to be ratcheted into the blocking tier once the backlog is cleared. - .github/workflows/e2e.yml: nightly schedule (03:47 UTC) so drift landing between PRs is caught within a day; LLM-gated specs run on schedule only when the ANTHROPIC_API_KEY secret is configured, else they self-skip. - docs/e2e-ci.md: document the accessibility scans, the nightly run, and how to use the uploaded playwright-report artifact (retries + traces were already configured; no config change needed there). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> | ||||
b66c0cbc | fix(a11y): accessible names for icon button and unlabeled inputs | Amalanand Muthukumaran | 2026-07-25 | ↗ GitHub |
commit bodyFixes the 3 critical axe violations flagged by e2e/accessibility.spec.ts
on its first CI run (upstream PR #243):
- /assistant, `button-name`: the icon-only send/stop button in ChatInput
(ArrowRight/Square icon, no text) had no accessible name. Add a
state-aware aria-label ("Send message" / "Stop response"), matching the
aria-label convention of its siblings (AddDocButton, workflows button).
- /projects, `label`: the select-all checkbox in the table header row had
no label/aria-label. Add aria-label="Select all projects".
- /tabular-reviews, `label`: same select-all header checkbox pattern.
Add aria-label="Select all reviews".
No markup or styling changes - aria-label attributes only.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| ||||
4669cc01 | fix(e2e): keep the HTML report when the CI reporter is set to "github" | Amal | 2026-08-05 | ↗ GitHub |
commit bodyWHY THIS MATTERS
docs/e2e-ci.md ("Failure artifacts") tells a maintainer debugging a red
nightly run to download the `playwright-report` artifact and open it with
`npx playwright show-report playwright-report`. But the config set
`reporter: "github"` on CI, and naming ANY reporter REPLACES Playwright's
default `html` reporter rather than adding to it - so `playwright-report/`
was never written, the artifact upload had nothing to ship (it only
survived because `if-no-files-found: ignore` hid the gap), and the
documented debugging flow failed at exactly the moment it was needed:
when a nightly run is red and someone needs the per-spec traces.
WHAT IS A PLAYWRIGHT REPORTER (AND WHY "REPLACES", NOT "ADDS")
A reporter is a plugin that consumes test events and renders them
somewhere: `list` prints one line per test to the terminal, `github`
emits workflow annotations (`::error file=...`) that GitHub renders
inline on the PR diff, and `html` writes a browsable report with
screenshots and traces to `playwright-report/`. When no `reporter` is
configured, Playwright uses `html` on CI by default - but the `reporter`
option is a full override, not a merge. Setting it to a single reporter
silently drops the default. To get several outputs you must list them
all explicitly as an array of tuples:
reporter: process.env.CI
? [["github"], ["html", { open: "never" }]]
: "list",
Each entry is `[name]` or `[name, options]`.
HOW THE FIX WORKS
CI now runs BOTH reporters: `github` keeps the inline PR annotations,
and `html` regenerates `playwright-report/` so the workflow's existing
`actions/upload-artifact` step (which already lists `playwright-report/`
in its `path`) has real content to upload. The `open: "never"` option
matters on CI: the html reporter's default (`on-failure`) tries to
launch a local browser to display the report after a red run, which is
useless on a headless runner. The workflow runs `npx playwright test`
from the repo root, so the reporter's default output folder
(`playwright-report/` relative to the config) lands exactly where the
artifact step looks. Local runs keep the terse `list` reporter.
Verified: `npx tsc --noEmit` passes and `CI=1 npx playwright test
--list` loads the config and enumerates all 31 tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| ||||
3294e914 | docs(e2e): correct stale spec counts in the workflow's comments | Amal | 2026-08-05 | ↗ GitHub |
commit bodyWHY THIS MATTERS
Two comments in .github/workflows/e2e.yml still said a keyless run
"passes the other 23 specs". That was true of an earlier revision of
this branch, but the suite has since grown (4 accessibility specs were
added): it now has 31 specs, of which 4 are LLM-gated, so a keyless run
passes 27. Stale numbers in CI comments are worse than no numbers: a
maintainer eyeballing a green keyless run against the comment ("23
passed? but I see 27...") is left wondering whether extra specs
accidentally ran with a leaked key, or whether the skip mechanism is
broken - the comment turns a healthy run into a false alarm. The
companion doc (docs/e2e-ci.md) already states 31/27; the workflow now
agrees with it.
WHAT IS COMMENT DRIFT
Comments are not checked by any compiler or test, so they rot silently
when the code they describe changes - here, the spec count changed in a
later commit on the same branch and nothing forced the comment to keep
up. The practical defenses are (a) keeping numbers in as few places as
possible and pointing everywhere else at that one place, and (b)
sweeping comments whenever the quantity they cite changes. This commit
applies (b); both comments also point at docs/e2e-ci.md, which remains
the authoritative source for expected pass/skip totals.
HOW THE FIX WORKS
Pure comment edits, no behavioral change:
- the workflow header now reads "of the 31 specs, the 4 LLM-dependent
ones ... skip themselves ... so a keyless run passes the other 27
specs";
- the ANTHROPIC_API_KEY env comment now reads "still green on the other
27 of the 31 specs".
Verified the file still YAML-parses (python3 -c "import yaml;
yaml.safe_load(...)") and that `CI=1 npx playwright test --list`
reports "Total: 31 tests", matching the corrected figures.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| ||||
1b00314d | Merge pull request #256 from amal66/olp-pr/stack-tests-ci | Will Chen | 2026-08-06 | ↗ GitHub |
[Testing 16] ci: run the RLS/stack integration suite on every PR | ||||
b0ee67e4 | Merge pull request #243 from amal66/olp-pr/e2e-hardening | Will Chen | 2026-08-06 | ↗ GitHub |
[Testing 20] test(e2e): accessibility scans + nightly run (stacked on #220) | ||||
Capture this PR into my fork
Download a Markdown prompt that tells Claude how to port every
commit in this PR into your working tree. Run it via
claude -p < capture-pull-22.md from
inside the repo you want the changes in.