[pull] main from Open-Legal-Products:main

✅ merged · #22 · admariner/mike ← Open-Legal-Products/mike · opened 1mo ago by pull[bot] · merged 1mo ago by pull[bot] · +334-15 across 10 files · ↗ on GitHub

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 body
The 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 body
Addresses 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 body
WHY 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 body
Fixes 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 body
WHY 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 body
WHY 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.

⬇ Download capture-pull-22.md