Skip to content

test(e2e): local-backend Playwright suite for workplace, with advisory CI - #476

Merged
camreeves merged 18 commits into
developfrom
e2e/ci-verify
Aug 5, 2026
Merged

test(e2e): local-backend Playwright suite for workplace, with advisory CI#476
camreeves merged 18 commits into
developfrom
e2e/ci-verify

Conversation

@camreeves

@camreeves camreeves commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Adds an end-to-end test suite for workplace that runs only against a local backend, the
isolated stack to run it against, and an advisory CI job on a self-hosted runner.

Nothing here gates merges. See Not a gate.

What's in it

e2e/support/ The engine: headless PKCE mint → storageState, a real-login driver, worker-scoped auth fixtures, idempotent API-driven seeding, stack preflight
e2e/stack/ A self-contained 10-service PlaceOS deployment — own compose project, own volumes, ports 9443/9080 — so it coexists with your own local stack and cannot disturb it
apps/workplace/e2e/local/ 3 specs: boot + org data, real login (scope and sub surviving refresh), a non-admin booking a desk through the full UI
E2E_USER_STORIES.md The coverage contract — what's covered, what isn't, and why
.github/workflows/e2e-advisory.yml Advisory CI on a self-hosted macOS runner
config/proxy.conf.js Only change to shared code — makes the dev-server proxy target env-driven. Defaults unchanged.
e2e/stack/up.sh                                  # backend, seeded, from nothing
export E2E_BACKEND_URL=https://localhost:9443
bunx playwright test --config apps/workplace/playwright.config.ts

7 passing, ~3 min in CI, ~26s locally. Verified green from a genuine cold start
(up.sh --fresh, volumes destroyed) and on the runner.

Local backend only

assertLocalOnly() throws at config load if E2E_BACKEND_URL/E2E_APP_URL resolve to anything
but a loopback host — an allowlist, not a prod blocklist, because a suite that creates and deletes
real data should not be one typo from doing it to a deployment.

The only surface needing anything external is room/calendar events, which is marked out of scope
and must never enter a gate. Desks, lockers, parking and visitors all work against a placeholder
tenant with no outbound calls.

Not a gate

Deliberately advisory until it has earned trust:

  • No pull_request trigger. This repo is public and the runner is self-hosted, so a fork PR
    could run arbitrary code on a machine on an internal network. Every trigger requires write access.
  • Not a required status check, and nothing in the workflow can make it one.
  • Cannot delay builds. build.yml runs on ubuntu-latest, a different runner pool, and
    Actions has no cross-workflow needs.
  • The job is named "advisory — does not block builds" so a red X beside the build jobs can't be
    misread.
  • Triggers are staged. Stage 1 (this PR): nightly + manual + e2e/** — nobody sees a new check
    on their commits. Stage 2, once the nightly has a record: add push: develop. Three-line change.

Note the nightly only starts once this is on developschedule fires from the default branch
only. That's what breaks the chicken-and-egg of proving it before merging it.

Two real bugs found, both filed

  • PPT-2642 — one burst of concurrent
    POST /bookings permanently poisons staff-api's connection pool; every later booking-create
    returns 500 until the service restarts. Reproducer kept at
    e2e/support/repro/reg09-concurrent-bookings.ts.
  • PPT-2643 — the booking form discards
    typed input while still initialising (title / All Day / Require locker revert).

Neither is fixed here: PPT-2642's likely fix is in pg-orm, shared by every service in a
transactional path; PPT-2643's fix embeds a UX decision in a service used by every booking flow.

⚠️ bookDeskViaUI works around PPT-2643, so this suite no longer detects it. When it's fixed,
remove the workaround and replace it with a spec asserting input survives init. Flagged in the
contract.

Review notes

Worth a look at config/proxy.conf.js (shared dev config) and the workflow. The rest is new files.

Things learned the hard way, all documented in code comments rather than lost:

  • GET /bookings is caller-scoped — an admin sees none of another user's bookings, so a leak
    check run as admin proves nothing.
  • The list query param is type, not booking_type.
  • Desks are Zone metadata, not systems.
  • A fresh authority gets a relative login_url, which ts-client resolves without the port —
    dead-ends the login redirect on any non-443 deployment. seed.ts patches it.
  • ES 7.17.6 crashes under cgroup v2; pinned to 7.17.28. Do not downgrade.
  • frontend-loader is required even though the dev server serves the SPA — it supplies /login.

🤖 Generated with Claude Code

camreeves and others added 11 commits July 30, 2026 11:46
Adds an end-to-end suite that runs only against a local PlaceOS backend, plus an
isolated stack to run it against and an advisory CI workflow.

- e2e/support: headless PKCE mint + storageState, real-login driver, worker-scoped
  auth fixtures, idempotent API-driven seeding, stack preflight
- e2e/stack: self-contained 10-service PlaceOS deployment on its own compose
  project and ports, so it coexists with a developer's own stack
- apps/workplace/e2e/local: boot, real login (incl. scope/sub surviving refresh),
  and a non-admin desk booking driven through the full UI
- E2E_USER_STORIES.md: the coverage contract
- config/proxy.conf.js: make the dev-server proxy target env-driven (defaults
  unchanged)

The suite refuses any non-loopback backend via an allowlist that throws before a
browser launches. CI is ADVISORY and must not gate merges until the suite has a
track record — see the CI section of E2E_USER_STORIES.md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first GitHub Actions run failed at bring-up with a single line —
"container placeos-e2e-elastic-1 is unhealthy" — and no way to see why.

- Elasticsearch had `bootstrap.memory_lock: true`, which requires an unlimited
  memlock rlimit or ES refuses to boot and the container exits. Docker Desktop
  grants that by default, so it only failed in CI. Turned off (production tuning,
  worthless to a throwaway test stack) and the ulimits set either way.
- up.sh now dumps `compose ps` and logs for every service when bring-up fails, so
  the step output explains itself instead of needing an artifact. `set -E` is
  required for that ERR trap to be inherited by shell functions — verified by
  breaking a service on purpose.
- The workflow's log collection enumerated a hand-picked service list that did not
  include elastic, so the one log that mattered was the one not captured. It now
  collects every service plus `compose ps`.
- Bounded the health wait (`--wait-timeout 300`) so a stuck container fails
  clearly rather than running to the job timeout.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7.17.6 (what PlaceOS/local pins) bundles a JDK with the cgroup v2 NPE bug. Its
launcher dies in JvmOptionsParser -> DefaultSystemMemoryInfo with "Cannot invoke
CgroupInfo.getMountPoint() because anyController is null" before the JVM starts,
so no ES_JAVA_OPTS workaround is possible. GitHub runners use cgroup v2; Docker
Desktop's VM does not, which is why this only failed in CI.

Staying on the 7.x line keeps client compatibility with rest-api and search-ingest.
Verified locally on 7.17.28: search-ingest builds its indices, suite 7/7.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first green CI run was green but reported both desk specs as flaky — failing
on attempt 1 with "the confirm dialog did not open", passing on retry.

The booking form is rebuilt when its async initialisation finishes, and the
rebuild restores defaults: title -> "Booking", All Day -> off, Require locker ->
on. It is a RACE, not a step. On a warm run it lands before we touch anything; on
a cold one it lands mid-flow and silently discards our input. Locally that
surfaced as a booking under the wrong title; in CI as an unopenable confirm
dialog, because a reverted All Day leaves the default slot, which on a slow run
has already passed and makes the form invalid with no visible error.

Probing showed the values sometimes survive desk attachment and sometimes do not,
so re-ordering cannot fix it. Re-applying inside a retrying block converges
whenever the rebuild fires, without depending on an internal ready signal.

Note this mitigates a race in the app, it does not fix one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Log all five CI runs honestly, including that run 1's diagnosis was wrong and
  that run 3 was "green" while reporting two flaky specs.
- REG-09: note that DB::ConnectionLost has NOT been seen in CI, but CI runs 2
  workers vs 4 locally, so the quiet record may just be lower concurrency rather
  than the problem being absent.
- REG-10 (new): the booking form discards input while still initialising. A real
  user can hit this. bookDeskViaUI now works around it, which means the suite no
  longer detects it — so it needs a row of its own rather than being buried in a
  test helper.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- runs-on: [self-hosted, placeos-e2e]
- Drop the `pull_request` trigger. This repo is PUBLIC and the runner is a machine
  on an internal network, so a fork PR could run arbitrary code on it. Remaining
  triggers (schedule, dispatch, push to e2e/**) all require write access. Costs
  nothing today since the check is advisory and not a PR gate.
- Add a reclaim step: a self-hosted machine is not a fresh VM, and a previous
  aborted run can leave the stack up or port 4214 held.
- Drop the vm.max_map_count bump — required on GitHub-hosted Linux, meaningless on
  macOS where the value lives inside Docker Desktop's VM.
- Move the nightly to 01:10 UTC and note that `schedule` only fires from the
  default branch, so the track record does not start until this is on develop.
- Add e2e/stack/SELF_HOSTED_RUNNER.md: the runbook, led by the fact that no inbound
  access is needed, with a pre-flight connectivity check and the macOS gotchas
  (Docker Desktop needs a GUI session, sleep kills nightlies, service PATH).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Nobody needs to reach the runner: committers push to GitHub, the runner collects
  work over its own outbound connection. The machine can sit on a network most of
  the team cannot reach and CI still works for everyone.
- Setting up over SSH alone cannot complete two steps: Docker Desktop is a GUI app
  that will not start without an active GUI session, and svc.sh installs a launchd
  agent that belongs to one. Do those at the machine once, then SSH is fine.
- Troubleshooting: being unable to ping/SSH the box is expected (macOS Remote Login
  off by default, ICMP dropped, possible wifi client isolation) and unrelated to CI.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first self-hosted run spent 9.1 min uploading node_modules to GitHub's cache
and 3.1 min saving the bun cache, out of 19.2 min total — while the suite itself
took 1.0 min. actions/cache, setup-node and setup-bun are the right answer on a
hosted runner that starts from a bare VM; on a persistent machine the workspace and
toolchain are already present, so shipping them to a remote cache over a slow
uplink costs more than everything else combined.

Toolchain is now provisioned once on the machine (brew: colima, docker,
docker-compose, bun, node@24), all resolvable from the runner service PATH.

Also:
- Fix a real concurrency bug: a scheduled run and a develop push share
  github.ref, so with a ref-only group plus cancel-in-progress a push would have
  CANCELLED an in-flight nightly, destroying the run the track record depends on.
  Keyed on github.event_name as well.
- Name the job "advisory — does not block builds": the job name is what appears in
  the Checks list beside the build jobs, so a red X cannot be misread.
- Stage the trigger rollout. `develop` is deliberately NOT enabled yet — it is the
  high-value trigger but it also puts a new check on everyone's commits, which
  should be earned once the nightly has a record. Three-line change when ready.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both findings investigated to root cause and filed against PPT, assigned to Cam.

PPT-2642 (REG-09) — worse than the flake suggested. One burst of concurrent
POST /bookings permanently poisons staff-api's connection pool; every later
booking-create returns 500 with "There is an existing transaction in this
connection" until the service restarts. Verified: serial requests are always clean
(including 409/422 raised inside the transaction), a concurrent burst looks healthy
itself, and everything after it fails. Reproducer kept at
e2e/support/repro/reg09-concurrent-bookings.ts rather than only in the ticket.

PPT-2643 (REG-10) — booking-form.service.ts newForm() defers itself until the
current user loads, then resets the form, discarding anything typed in the meantime.
One race, two symptoms: locally it ate the title, in CI it ate All Day (leaving the
default 5-minute slot, already past on a slow run, silently invalidating the form).

Neither is a PR: REG-09's likely fix is in pg-orm, shared by every PlaceOS service
in a transactional path; REG-10's fix embeds a UX decision (preserve dirty input vs
gate the form until ready) in a service used by every booking flow.

Noted in the contract that the suite no longer detects REG-10 — bookDeskViaUI works
around it — so the workaround must be removed when it is fixed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The runbook described the setup I planned, not the one that exists. Rewritten from
what actually works, with the corrections that cost time:

- Colima, not Docker Desktop (no GUI session, startable over SSH)
- brew's docker-compose needs cliPluginsExtraDirs or `docker compose` never resolves
- osx-x64 (the machine is Intel), /usr/local not /opt/homebrew
- svc.sh is generated by config.sh, not shipped in the tarball
- a slow link looks exactly like a blocked host — retry before blaming the network
- records the measured 2.8 min job time and why remote caching was removed
- documents the reboot limitation honestly rather than implying it is handled

Also fix the REG-09 reproducer's import, broken when it moved into
e2e/support/repro/ — verified it runs from the repo root.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Caught reviewing my own diff — the README still attributed vm.max_map_count to
Docker Desktop's VM after the switch to Colima.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

Deployment failed with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/placeos?upgradeToPro=build-rate-limit

@camreeves
camreeves requested review from MrYuion and stakach July 30, 2026 05:13
A nightly result nobody sees is not a result. Adds a failure notification using
the same chat integration PR Flow already uses, so there is nothing new to
configure — repoint CHAT_URL to change the recipient.

Only fires on failure. A green run stays silent, or the signal gets tuned out
within a fortnight. continue-on-error so a broken webhook can never turn a green
run red.

Flaky runs pass by conclusion, so nothing notifies. The step summary now says so
explicitly instead — that is the number that tells us whether the suite can be
trusted yet, and the easiest one to quietly stop looking at.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
frontend-templates Ignored Ignored Preview Aug 5, 2026 6:22am

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@MrYuion MrYuion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for putting this together — the isolation work, worker-scoped auth, API-backed assertions, and failure artifacts are a strong foundation. I found a few issues I think should be addressed before merging.

Requested changes

1. [P1] Pin the third-party action used on the self-hosted runner

.github/workflows/e2e-advisory.yml:209 uses fjogeleit/http-request-action@master. That mutable ref executes on an internal self-hosted runner and receives STATUS_URL and CHAT_URL as inputs. A compromised or unexpectedly changed upstream ref could therefore execute code inside the runner's network and access those inputs.

Please pin a reviewed full commit SHA, or replace this step with a small curl invocation. An explicit least-privilege workflow permissions block would also reduce exposure.

2. [P2] Replace the PPT-2643 workaround with its regression test

bookDeskViaUI repeatedly reapplies the title and checkbox values to work around form initialization rebuilding the model. PPT-2643 has now been fixed on develop by #478, so this workaround is stale and means the E2E suite will continue to pass if that fix regresses.

As the PR description already proposes, please remove the workaround and add an E2E assertion that input entered during initialization survives.

3. [P2] Keep the mock project genuinely backend-free

The config installs globalSetup unconditionally, and that setup always calls assertStackUp(). Consequently:

bunx playwright test --config apps/workplace/playwright.config.ts --project=mock

still fails when the PlaceOS stack is absent, despite being documented as “no backend.” Please make preflight conditional on the selected local project, or model it as a setup-project dependency used only by local.

4. [P2] Fail the boot smoke test on unexpected API errors

boot.spec.ts collects every /api/ and /auth/ 4xx/5xx response but only logs the list. A regression in a required endpoint can therefore leave the smoke test green as long as the shell still renders.

Please explicitly allowlist the known calendar-related failure and fail on unexpected API/auth errors, or assert the required boot requests individually.

5. [P2] Pin the backend inputs used to establish the nightly track record

Most PlaceOS services default to latest, and a fresh www volume lets frontend-loader repopulate www-core. This means a nightly result can change without any frontend commit, making it difficult to distinguish test flakiness from moving backend inputs.

Please pin known service tags/digests and a known frontend source revision, then update them intentionally.

6. [P2] Preserve the existing Firefox and WebKit coverage

The previous config ran the landing spec in Chromium, Firefox, and WebKit. Both replacement projects use Desktop Chrome, removing the other two browsers and breaking existing --project=firefox / --project=webkit invocations.

Unless that reduction is intentional and agreed, please retain browser variants for the mock smoke test.

7. [P3] Correct the CI documentation

e2e/README.md says the workflow runs on PRs into develop at 15:10 UTC. The workflow deliberately has no pull_request trigger and schedules 01:10 UTC. The README should match the workflow, especially because the absence of a PR trigger is a security decision.

Validation performed

I reviewed head a06d1bbe7 and also tested its merge result against current develop:

  • clean automatic merge
  • TypeScript check passed after postinstall
  • Playwright config loaded successfully and discovered all 7 tests
  • YAML, Bash syntax, and diff whitespace checks passed

I did not start the ten-service PlaceOS stack, so I did not execute the full E2E suite.

camreeves and others added 2 commits August 5, 2026 14:26
…ject, boot assertions

Merges develop to pick up #478 (the PPT-2643 fix) and applies the review
feedback on #476.

CI hardening. The job now declares `permissions: contents: read` and checks
out without persisting credentials: the repository default is `write`, and
this is the only job on a self-hosted machine, where a token written to disk
outlives the run. The failure notification drops
`fjogeleit/http-request-action@master` for plain curl — same request, but a
mutable ref should not execute on a box inside the internal network. The six
other call sites are the same pattern on GitHub-hosted runners and are left
for a repo-wide change.

Preflight becomes a setup project that only `local` depends on, instead of a
`globalSetup` that ran for every invocation. `--project=mock` is documented
as needing no backend and now genuinely does not — verified by running it
with the stack down. A missing stack also reports as a named failing test
rather than a runner crash.

The boot smoke test asserts on unexpected server errors instead of only
logging them; a required endpoint could previously start failing while the
test stayed green. The tolerated set is derived from an observed CI run
rather than from documentation, scoped to 5xx and to same-origin API paths.

Records the backend inputs (resolved image IDs, www-core HEAD) in the job
summary so a changed nightly is attributable, and pins keydb by multi-arch
digest. The PlaceOS services stay on ${PLACEOS_TAG} deliberately: catching
backend regressions is what an advisory nightly is for, and PLACEOS_TAG is
already the knob for pinning when bisecting.

Documents that mock is Chromium-only on purpose, and corrects the CI section
of the README, which described a `pull_request` trigger that deliberately
does not exist.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@camreeves

Copy link
Copy Markdown
Contributor Author

Thanks — this was a genuinely useful review, and one of your points turned out to be more
important than either of us thought. Every factual claim checked out. Pushed as c0c4c69,
which also merges develop in.

Done

Least-privilege CI. You were right, and the sub-point you tacked on the end is the bigger
half: default_workflow_permissions on this repo is write, so with no permissions: block
this job was running with a write-capable token, and actions/checkout persists it to
.git/config — on the one runner that isn't a fresh VM. Added permissions: contents: read
and persist-credentials: false.

The notification step is now plain curl rather than a pinned SHA. Same request, no
third-party code, and nothing to bump later. Worth noting for context: that action with that
exact secrets payload already runs six times across build.yml and pull-request.yml, so it
isn't new exposure — what is new is running it on a machine inside the internal network,
which is why it's worth not extending the pattern here. I've left the other six alone; that's a
repo-wide change and doesn't belong in this PR. If you want it done, MrYuion/git-publish-subdir-action@develop
is arguably the better target of the two, since it receives GITHUB_TOKEN.

Backend-free mock project. Fixed the way you suggested second, because the first isn't
actually available: Playwright never exposes the CLI project filter to globalSetup (it hands
over the full config, and cliProjectFilter isn't on it), so a conditional guard would have to
sniff process.argv and would break under --ui and the VS Code extension. Preflight is now a
setup project that only local depends on. Verified by running --project=mock with the stack
down — passes. Bonus: a missing stack now surfaces as a named failing test instead of a runner
crash.

Boot smoke test. Agreed, it was only logging. It now fails on unexpected server errors.
Three deliberate narrowings: 5xx only (a 4xx during boot is often benign — optional-resource
probes, permission-shaped answers for a non-admin), same-origin only (the old substring test for
/api/ would match any third-party URL containing that segment), and the tolerated set derived
from an actual CI run rather than from the README — the observed list is exactly one entry,
500 GET /api/staff/v1/calendars. Writing that allowlist from the docs would have been writing
it blind, which is how you get flakes on a suite that's trying to earn a track record.

Backend inputs. Right that a nightly can change with no frontend commit. I've gone with
recording rather than pinning: the job summary now lists the resolved image IDs and the
www-core HEAD, so a changed result is attributable in seconds. keydb is pinned by multi-arch
digest — it's not the system under test, so a moving tag there is pure noise (it had no tag at
all; upstream publishes only arch-specific version tags, so a digest is the only pin that
doesn't break either the Intel runner or an Apple Silicon laptop).

I'd push back on pinning the PlaceOS services themselves. They ship together on a rolling tag,
and an advisory nightly against current backends catching a regression before a customer does is
worth more than reproducibility we can get on demand — PLACEOS_TAG is already threaded through
every service, so pinning while bisecting is one env: line. Happy to be overruled if you'd
rather have the determinism.

README. Corrected. You were right to flag it above cosmetic: it claimed a pull_request
trigger eighteen lines above the section explaining why that trigger must never exist, which is
pointing straight at the trap.

Not done, with evidence

The PPT-2643 workaround stays, because #478 does not fix this route.

This is the interesting one. Two things:

First, sequencing — this branch didn't contain a0360486 (it was 154 commits behind), so
removing the workaround before merging develop would just have turned desk-booking red for the
wrong reason. That part's now moot, develop is merged.

Second, and this is the part worth your attention: with your fix present I removed the
converging block and ran the suite. Desk booking still fails, at all_day.isChecked() — the
same assertion and the same symptom as before the fix. I then reverted booking-form.service.ts
to its pre-fix state and ran again: it fails identically. So the flow is a real detector, and
the app behaviour on this path simply hasn't changed.

Best guess at why, though you know that service far better than I do: #478 guards newForm's
deferred re-entry, but loadForm and resetForm keep the same unguarded pattern, loadForm
runs first on /#/book/desk/form, and the unconditional model.set() / form().reset() in
newForm is untouched.

I also tried the spec you asked for — type during initialisation, assert it survives — and
deliberately did not keep it. It passed with and without the fix, because the rebuild is a
race that a warm local run wins before you can type; I tried forcing the condition by holding the
API responses and it still couldn't be made to fail. A spec that cannot fail is worse than no
spec. The honest detector is the flow itself with the workaround removed, which is a two-line
change the day the app path is genuinely fixed. REG-10 in E2E_USER_STORIES.md now records all
of this, including the measurement.

Might be worth reopening PPT-2643, or splitting the remaining paths into a follow-up.

Note for whoever merges

#477 is stacked on this branch, so it'll want develop merged into it too once this lands.

Review follow-up on #476:

- The boot spec asserted on collected 5xx as soon as the topbar rendered,
  but the shell renders before the org/zone calls finish — a slow 500
  could land after the check and leave the test green. Wait for
  networkidle first so in-flight boot requests are counted. WebSockets do
  not count against networkidle, so the realtime channel cannot hang it;
  the 90s test timeout backstops the wait regardless.
- preflight's "stack is down" message still pointed at `cd local &&
  ./placeos start` — a developer's personal stack, not this suite's.
  Now points at `e2e/stack/up.sh`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@camreeves

Copy link
Copy Markdown
Contributor Author

Both changes in be3ce21 are right — thanks, the first one fixes a real hole in what I pushed.

The networkidle wait. You're right that the assertion was running too early: the shell
renders before the org/zone calls finish, so a slow 500 could have landed after the check and
left it green — an assertion that only catches fast failures isn't much of one. Checked the two
things that could have made this a bad trade and both hold up:

  • Does it hang? No. Three consecutive boot runs, 2.8–3.1s each, no timeouts. If the realtime
    WebSocket counted against networkidle we'd be seeing the 90s backstop instead, so your note
    about that is borne out empirically.
  • Does the assertion still bite? Yes — I red-checked it rather than assuming. Forcing a 500 on
    /api/engine/v2/systems fails the spec with the three offending entries listed, while the
    tolerated 500 GET /api/staff/v1/calendars is still correctly ignored.

The preflight hint. Straightforwardly right, and my mistake — it was pointing developers at
cd local && ./placeos start, which is their own personal stack rather than this suite's
isolated one. Following that message would have started the wrong thing and left the suite
failing for a reason the message had just ruled out.

Full suite is 8/8 green locally on your head, and the advisory run on be3ce21 is green on the
runner.

One thing I noticed while probing that's worth writing down, though I'd leave it as-is for now:
the set of API calls made during boot varies between runs. /api/staff/v1/bookings and
/api/staff/v1/calendars appear in some runs and not others, depending on how far the landing
page gets. It doesn't affect correctness — a tolerated entry never fails and your networkidle
wait widens the window — but it does mean the tolerated list describes "what boot can do", not
"what boot always does". Worth remembering if that allowlist ever looks stale.

Also saw you took the curl swap repo-wide in 9fc04af and 82695eb, including your own
publish action. Nice — that closes the bit I'd deliberately left out of scope here.

Which leaves PPT-2643 as the only open thread: the workaround is still load-bearing, since
removing it with #478 present still turns desk booking red on the same assertion. Your call
whether that's a reopen or a follow-up ticket.

An earlier note claimed PPT-2643's fix did not cover the desk-booking route,
on the strength of the suite going red when the converging block in
bookDeskViaUI was deleted. That was wrong twice over, and both errors are
recorded so the next person does not repeat them.

The first red was a Playwright strict-mode violation rather than a reverted
value: opening the desk-select modal puts a second "All Day" checkbox in the
DOM, bound to the same form field, so an unscoped locator matched two
elements and threw — with both of them checked. Scoping the locator to
desk-flow-form then broke it a second way, because setCheckbox returns
silently when its locator matches nothing, so a narrower scope became a
no-op and the form really was invalid.

The shipped helper is unchanged and stable: six consecutive full runs, 8/8,
at --retries=0. Whether the fix is complete remains unproven in either
direction — the race needs a slower machine than this one — so the block
stays and REG-10 stays blocked, now with the two dead ends written down and
a note that settling it needs throttled CPU or a unit test, not another
e2e attempt.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous note said there was no evidence the fix was incomplete. There
is; it just was not going to come from this suite. `newForm`'s protected
branch is never taken by the flows, because the current user is restored from
cache long before org data lands, and `loadForm` — which the flows call first
— had no capture at all.

The suite passing 8/8 either way is the finding, not a reassurance: REG-10
stays blocked once #479 lands, because this hardware cannot lose the race.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@camreeves

Copy link
Copy Markdown
Contributor Author

Correction on the PPT-2643 thread, since I got there via two wrong turns and the record should be straight.

My earlier evidence was wrong. I said removing the converging block turned desk booking red at all_day.isChecked(), and that this proved #478 had not covered the route. That red was a Playwright strict-mode violation: opening the desk-select modal puts a second "All Day" checkbox in the DOM (desk-filters, bound to the same field), so an unscoped locator matched two elements and threw — with both of them checked. Scoping the locator to desk-flow-form then broke it a second way, because setCheckbox returns silently when its locator matches nothing, so a narrower scope became a no-op and the form genuinely was invalid. Both experiments are reverted; the helper on this branch is unchanged.

The conclusion still holds, for a different reason. Reading the code rather than chasing the race: newForm's protected branch is never taken by the flows. The current user is restored from the localStorage cache ~50ms after bootstrap, while NewDeskFlowComponent.ngOnInit awaits org init plus a 300ms settle before calling loadForm('desk') and then newForm('desk'). So currentUserIsLoaded() is already true, the deferral is skipped, and the replay never runs — and loadForm, which goes first, had no capture at all.

Fixed in #479, with two unit specs seen red first. PPT-2643 reopened.

What this means for this PR: the converging block stays, and REG-10 stays blocked even after #479 lands. Not because the app is broken, but because this suite passed 8/8 in six consecutive runs with the bug present — the race needs initialisation to be slow relative to typing, and the runner is not slow enough to lose it. Your original request to replace the workaround with a real assertion is right in principle; it just cannot be honoured with an e2e assertion that only fails on hardware we do not have. The unit specs are the guard instead, and E2E_USER_STORIES.md now says so explicitly rather than leaving a to-do that reads as achievable.

Everything else from your review is addressed and CI is green, so this is ready for another look whenever you are.

@camreeves
camreeves merged commit 2126006 into develop Aug 5, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants