Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/actions/setup/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
name: Setup pnpm workspace
description: Install pnpm + Node, and install workspace dependencies with the pnpm store cache. Shared by every CI job that needs the repo installed. Callers must `actions/checkout` first — a local composite action's own definition can't be read off disk until the repo is checked out, so checkout can't live inside this action.

runs:
using: composite
steps:
- uses: pnpm/action-setup@v4
# version comes from the root package.json's "packageManager" field

- uses: actions/setup-node@v4
with:
node-version: 24
cache: pnpm

- run: pnpm install --frozen-lockfile
shell: bash
28 changes: 12 additions & 16 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ jobs:
name: Playwright E2E (offline sync, accessibility)
needs: test
runs-on: ubuntu-latest
timeout-minutes: 15
Comment thread
brianramseyau marked this conversation as resolved.
env:
HOST: 127.0.0.1
PORT: '3333'
Expand All @@ -40,16 +41,18 @@ jobs:
ELECTRON_SKIP_BINARY_DOWNLOAD: '1'
steps:
- uses: actions/checkout@v4
- uses: ./.github/actions/setup

- uses: pnpm/action-setup@v4

- uses: actions/setup-node@v4
- uses: actions/download-artifact@v4
with:
node-version: 24
cache: pnpm
name: shared-dist
path: packages/shared/dist

- run: pnpm install --frozen-lockfile
- run: pnpm --filter @everylist/shared build
- name: Cache Playwright browsers
uses: actions/cache@v4
with:
path: ~/.cache/ms-playwright
key: playwright-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }}

- name: Install Playwright Chromium
run: pnpm --filter @everylist/web exec playwright install --with-deps chromium
Expand All @@ -60,20 +63,13 @@ jobs:
name: Build image and smoke test
needs: test
runs-on: ubuntu-latest
timeout-minutes: 15
Comment thread
brianramseyau marked this conversation as resolved.
env:
# See test.yml's env block — this job doesn't build apps/desktop either.
ELECTRON_SKIP_BINARY_DOWNLOAD: '1'
steps:
- uses: actions/checkout@v4

- uses: pnpm/action-setup@v4

- uses: actions/setup-node@v4
with:
node-version: 24
cache: pnpm

- run: pnpm install --frozen-lockfile
- uses: ./.github/actions/setup

- uses: docker/setup-buildx-action@v3

Expand Down
127 changes: 90 additions & 37 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: Test

# Reusable lint/typecheck/test job called by ci.yml — the single source of
# Reusable lint/typecheck/test jobs called by ci.yml — the single source of
# truth for "does this commit pass the PR gate" (branch protection on main
# requires it). docker-publish.yml does NOT call this: it only runs after a
# commit is already on main, where this has already passed. See
Expand All @@ -10,57 +10,110 @@ name: Test
# each package's own test config: packages/shared (vitest --coverage) and
# apps/api (c8, scoped to app/**, see apps/api/.c8rc.json) both do; apps/web
# still needs its Vitest coverage config wired up (see PLAN_00_FOUNDATIONAL_PLAN.md's Phase 2
# status note). A plain `pnpm -r test` is already the gate once a workspace
# opts in, no separate coverage step needed here.
# status note). A plain `test` script per workspace is already the gate once
# a workspace opts in, no separate coverage step needed here.
#
# lint / build-shared / typecheck / test are split into separate jobs (rather
# than one job's sequential steps) so independent work runs in parallel —
# lint doesn't depend on the shared build, and the api/web/desktop/shared
# test suites don't depend on each other. typecheck and the test matrix both
# need packages/shared built first (apps/api and apps/web import its types
# via package.json "exports" -> dist/), so build-shared runs once and hands
# its output to both via an artifact instead of every job rebuilding it.

on:
workflow_call:

env:
# Dummy, non-secret values so `node ace test` can boot apps/api (see
# apps/api/start/env.ts) — CI's SQLite database is an ephemeral tmp file
# thrown away at the end of the run. Harmless on jobs that don't touch
# apps/api.
HOST: 127.0.0.1
PORT: '3333'
NODE_ENV: test
LOG_LEVEL: info
APP_KEY: ci-3B65vVbNzQY6nqACpAxWnZUX2ZPfL5pI
APP_URL: http://127.0.0.1:3333
SESSION_DRIVER: cookie
LIMITER_STORE: database
# No job here builds apps/desktop, so none of them have use for Electron's
# ~150 MB runtime binary — pnpm-workspace.yaml's onlyBuiltDependencies
# allows electron's postinstall to run (for apps/desktop's own dev/CI use),
# so without this every job here would pay for that download too. See
# PLAN_22_PHASE_DESKTOP_APP_ELECTRON.md §6.
ELECTRON_SKIP_BINARY_DOWNLOAD: '1'

jobs:
test:
name: Lint, typecheck, test
lint:
name: Lint

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: Splitting the single test job into seven named jobs changes the required status-check names that branch protection on main must reference

The old gate exposed one check, test / Lint, typecheck, test. After this change the reusable workflow produces test / Lint, test / Build packages/shared, test / Typecheck, test / Test (api), test / Test (web), test / Test (desktop), and test / Test (shared). Branch protection is configured in GitHub repo settings (ci.yml already notes it isn't expressible in the workflow file), so the required-check list has to be updated in the same merge — otherwise the gate either blocks every future PR (the old required name no longer matches any produced check) or, if the old entry is just removed, silently stops enforcing this gate on main. native-build.yml calls the same test.yml and is affected the same way (its needs: test jobs keep working, but the check names it fans out to change too).


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

runs-on: ubuntu-latest
env:
# Dummy, non-secret values so `node ace test` can boot apps/api (see
# apps/api/start/env.ts) — CI's SQLite database is an ephemeral tmp
# file thrown away at the end of the run.
HOST: 127.0.0.1
PORT: '3333'
NODE_ENV: test
LOG_LEVEL: info
APP_KEY: ci-3B65vVbNzQY6nqACpAxWnZUX2ZPfL5pI
APP_URL: http://127.0.0.1:3333
SESSION_DRIVER: cookie
LIMITER_STORE: database
# This job never builds apps/desktop, so it has no use for Electron's ~150 MB runtime
# binary — pnpm-workspace.yaml's onlyBuiltDependencies allows electron's postinstall to
# run (for apps/desktop's own dev/CI use), so without this every job here would pay for
# that download too. See PLAN_22_PHASE_DESKTOP_APP_ELECTRON.md §6.
ELECTRON_SKIP_BINARY_DOWNLOAD: '1'
timeout-minutes: 10
steps:
- uses: actions/checkout@v4
- uses: ./.github/actions/setup
# Deliberately doesn't depend on build-shared: today's ESLint configs enable no
# type-aware rules, so a missing packages/shared/dist doesn't surface as a lint
# error. If a type-aware rule (e.g. typescript-eslint's recommended-type-checked)
# is ever turned on, this job will need build-shared as a dependency.
- run: pnpm -r lint

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SUGGESTION: lint now runs without build-shared having produced packages/shared/dist

This is fine today — the workspaces' ESLint configs only enable non-type-aware rules (apps/web/eslint.config.js sets projectService: true for .svelte* files but no type-checked rules are enabled, so a missing dist doesn't surface as a lint error). It is a latent coupling though: apps/api and apps/web import @everylist/shared via exports → dist/, so if a type-aware rule (e.g. recommended-type-checked) is ever enabled, the now-independent lint job would start failing on the missing build. Worth a one-line note so nobody either re-serializes lint on build-shared or breaks it accidentally later.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.


- uses: pnpm/action-setup@v4
# version comes from the root package.json's "packageManager" field

- uses: actions/setup-node@v4
build-shared:
name: Build packages/shared
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- uses: actions/checkout@v4
- uses: ./.github/actions/setup
- run: pnpm --filter @everylist/shared build
- uses: actions/upload-artifact@v4
with:
node-version: 24
cache: pnpm
name: shared-dist
path: packages/shared/dist
retention-days: 1

- run: pnpm install --frozen-lockfile
typecheck:
name: Typecheck
needs: build-shared
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- uses: actions/checkout@v4
- uses: ./.github/actions/setup
- uses: actions/download-artifact@v4
with:
name: shared-dist
path: packages/shared/dist
- run: pnpm -r typecheck

# apps/api and apps/web both import types from @everylist/shared via
# its package.json "exports" (dist/), so it must be built before
# typecheck/test can resolve it — lint doesn't need this.
- run: pnpm --filter @everylist/shared build
test:
name: Test (${{ matrix.workspace }})
needs: build-shared
runs-on: ubuntu-latest
timeout-minutes: 15
strategy:
fail-fast: false
matrix:
workspace: [api, web, desktop, shared]
steps:
- uses: actions/checkout@v4
- uses: ./.github/actions/setup
- uses: actions/download-artifact@v4
with:
name: shared-dist
path: packages/shared/dist

- run: pnpm -r lint
- run: pnpm -r typecheck
- name: Cache Playwright browsers
if: matrix.workspace == 'web'
uses: actions/cache@v4
with:
path: ~/.cache/ms-playwright
key: playwright-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }}

- name: Install Playwright Chromium
if: matrix.workspace == 'web'
# apps/web's component tests run in a real headless browser
# (vitest-browser-svelte), so `pnpm -r test` below needs it present.
# (vitest-browser-svelte), so its test script below needs it present.
run: pnpm --filter @everylist/web exec playwright install --with-deps chromium

- run: pnpm -r test
- run: pnpm --filter @everylist/${{ matrix.workspace }} test
21 changes: 19 additions & 2 deletions apps/api/tests/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,27 @@ export const runnerHooks: Required<Pick<Config, 'setup' | 'teardown'>> = {
// demo_seed.js investigated in PR history). Calling boot() directly (not
// just make()) is what actually closes that race — make() alone only
// constructs the Kernel instance and returns before any scan happens.
//
// This is a warm-up, not a load-bearing step: if it throws (seen in CI —
// the same "Invalid command exported... Invalid URL" validation error,
// now surfacing deterministically instead of intermittently, for reasons
// that look environment-specific rather than related to this file), don't
// let it take the whole run down. Swallowing it here just means the
// original race this hook exists to close is back on the table for that
// run, not that anything is broken outright — migration:run's own loader
// registers ahead of the commands/ FsLoader that's actually throwing, so
// exec() below still finds it. Letting an uncaught rejection from this
// hook propagate instead hangs the whole process indefinitely rather than
// failing fast (Japa's global setup doesn't turn that into a clean exit),
// which is strictly worse than the race it's meant to prevent.
setup: [
async () => {
const ace = await app.container.make('ace')
await ace.boot()
try {
const ace = await app.container.make('ace')
await ace.boot()
} catch (error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SUGGESTION: Swallowing a now-deterministic ace.boot() failure masks a root cause that should be investigated rather than silenced

The comment notes the "Invalid command exported... Invalid URL" validation error is now surfacing deterministically (not intermittently) "for reasons that look environment-specific" — but no root cause is pinned down and nothing links to an issue tracking it. The catch also swallows every boot error, not just the transient race. Because migration:run's loader registers ahead of the throwing commands/ FsLoader, the suite still goes green while this error keeps firing, so a real command-metadata bug in apps/api/commands/* (e.g. demo_seed.ts / openapi_generate.ts) would be invisible here. That same FsLoader scan runs on production boot (docker/root/etc/cont-init.d/30-migrate / 35-demo-seed), where there's no catch — so a real registration bug would still surface (or silently no-op) in prod while tests pass. Worth narrowing the catch to the known transient error and/or filing a follow-up to root-cause the deterministic failure, rather than swallowing it indefinitely.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

console.warn('ace kernel warm-up boot failed, continuing without it:', error)
}
},
],
teardown: [],
Expand Down
Loading