test(config): harden client recovery paths - #427
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Three of the six new Electron scenarios assert a transient refresh status that races the desktop app's own 1s background emergency refresh. As written they will fail nondeterministically depending on how fast the renderer paints.
Reviewed changes — full initial review of the two commits on xuan/code-557; test-only, no production source touched.
- Normal-channel fault coverage (
packages/foundation/common/src/config/__tests__/core.test.ts) — snapshot 404 / 200-with-no-body must still persist the advancedhighWaterandtrustedpointer while leavinglkgunset, and a stale-intermediary pointer 200 must be rejected asreplaywith both in-memory state and the stored blob byte-identical. Traced both againstConfigCore.#refreshNormal; the assertions pin the behavior they claim. - Mobile emergency composition (
apps/mobile/src/runtime/config/__tests__/adapters.test.ts) — drivesConfigCoreover the realcreateConfigNetwork/createConfigStorageadapters through killSwitch(v1) → forcedMinimum(v2) → restart → release(v3) → restart-offline with the normal channel permanently down. The two restarts genuinely re-readMemoryAtomicStorage, so the persistence claim is real coverage. - Second HTTPS origin for the pilot E2E (
apps/desktop/e2e/config-canary/{dist-server,electron-app,fixture,state-file}.mts) —startEmergencyServeronEMERGENCY_PORT = PORT + 1000(port ranges are disjoint),readEmergencyStatesplit out behind astorageKeyparameter, and the bootstrap'semergencyEndpointswitched fromnullto a live origin. - Six new Electron scenarios (
apps/desktop/e2e/config-canary.e2e.mts) — kill switch during main-channel outage, newer document replacing it, survival across a reconstructed runtime, explicit release, equal-version equivocation rejection, and sticky release.
I checked whether enabling emergencyEndpoint perturbs the 12 pre-existing scenarios: it does not. emergencyMode starts at 'offline', so those launches just destroy the socket, the refresh errors, and the retry backs off to 60s; no pre-existing assertion reads emergencySupport or the emergency request log.
ℹ️ The E2E carrying these scenarios never runs in CI
e2e:config-canary appears only in apps/desktop/package.json:21 — no workflow in .github/workflows/ invokes it, unlike e2e:unpackaged, e2e:packaged, e2e:window-bounds, e2e:startup, and e2e:browser. Combined with the up-to-15-minute MAIN_VITE_CONFIG_BOOTSTRAP rebuild it requires, that means the emergency scenarios will realistically be exercised only when someone runs them by hand, which is also what makes the race above expensive: it will surface on an unrelated future run with no known-good baseline to compare against. Whether to gate this in CI (nightly, or on changes under src/config/) is your call, not something the diff can decide.
Technical details
# `e2e:config-canary` is manual-only
## Affected sites
- `apps/desktop/package.json:21` — the only reference to `e2e:config-canary` in the repo.
- `.github/workflows/ci.yml:129,166,180,186,215` — the E2E suites that *are* gated; config-canary is absent.
## Required outcome
- A deliberate decision on whether the emergency-recovery scenarios are CI-enforced or
documented as a manual pre-release gate. Either is defensible; silently manual is the
outcome to avoid.
## Open questions for the human
- Is the ~15 min desktop rebuild the blocker? If so, a nightly or `paths`-filtered job on
`packages/foundation/common/src/config/**` + `apps/desktop/src/main/config*.ts` would cover
the regression surface without taxing every PR.ℹ️ Nitpicks
apps/desktop/e2e/config-canary.e2e.mts—harness.emergency.requests.length = 0is reset in all five new scenarios but read in only one (line 226). Either assert on the log (see the equivocation comment) or drop the unused resets.apps/desktop/e2e/config-canary.e2e.mts—emergencyModeis assigned outsidewithLaunchwhilemodeis assigned inside it. Threading the emergency mode throughwithLaunchalongsidenextModewould make the reset impossible to forget.apps/desktop/e2e/config-canary/dist-server.mts:177— the emergency server hardcodes/v1/acme/desktop/emergency.json, whereasstartDistServerderives every path from the fixture (artifact.step.pointerPath). Deriving it fromfixture.target.brandIdkeeps the two servers consistent if the fixture's brand ever changes.apps/mobile/src/runtime/config/__tests__/adapters.test.ts:92— theit.each(['ios','android'])parameterization is inert for this case.osonly reachescontext, andcontextis read solely byevaluateSnapshot, which never runs here because the normal channel isrejectingFetch. Both parameterizations execute identical code; a plainitwould say the same thing in half the time.
Claude Opus ｜ 𝕏
| assert.equal(boundary.report.normal, 'error'); | ||
| assert.equal(boundary.report.emergency, 'updated'); |
There was a problem hiding this comment.
This asserts a transient refresh status that races the app's own background emergency refresh. startDesktopConfigRefresh() runs at app-ready before createDesktopWindow() (apps/desktop/src/main/index.ts:75) and schedules an emergency refresh 1s later (FIRST_EMERGENCY_REFRESH_DELAY_MS, apps/desktop/src/main/config.ts:38), while launchApp only waits for body to be visible. If that background refresh lands first it accepts the document and stores the ETag, so this explicit refresh() sends If-None-Match, the new server 304s (dist-server.mts:186), and report.emergency is 'not-modified'. Same exposure at lines 237 and 262.
| assert.equal(boundary.report.emergency, 'error'); | ||
| const emergency = boundary.info.emergency; | ||
| assert(emergency); | ||
| assert.deepEqual(emergency.disabledFeatures, []); | ||
| assert.equal(emergency.emergencyVersion, '3'); | ||
| assert.equal((await readEmergencyState(harness.home))?.raw, releaseRaw); | ||
| console.log('PASS equal-version emergency equivocation cannot replace explicit release'); |
There was a problem hiding this comment.
This scenario passes without the equivocating document ever being delivered — report.emergency === 'error' and unchanged persisted bytes are equally consistent with a 404, a path typo, or a dead server, which is exactly what the two 'offline' scenarios around it assert. Since anti-equivocation is the security property being claimed, add the delivery proof the kill-switch scenario already uses at line 226: await waitForRequest(harness.emergency, (request) => request.mode === 'equivocation' && request.status === 200);. The requests.length = 0 reset on line 273 is already set up for it.
|
Your Claude subscription has hit its usage limit. It resets at 11am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
1 participant
Summary
Verification
pnpm check:cipnpm test— 2,732 passed, 1 skippedpnpm test packages/foundation/common/src/config/__tests__/core.test.ts apps/mobile/src/runtime/config/__tests__/adapters.test.ts— 40 passedxvfb-run -a pnpm -F @linkcode/desktop e2e:config-canarypnpm -F @linkcode/mobile smoke:export— Android and iOS Hermes bytecode, source maps, routes, and bundled config sentinel validated54ce1fc855e12295a8dd1490463c9afac8e84a526f1e16340bcefe4f0fec8e392fa79670900ed6e80c159cd2a569814f6e5c0302059b7c5ffeb2c0a5707af3b4Boundaries
Electron restart coverage uses separate real app launches over shared atomic files, not abrupt OS process-kill injection. Mobile restart coverage reconstructs the core over the adapter seam with the frozen desktop-target fixture; the Hermes export is an app-entry bundle gate, not device/provider/native-module execution.
HQ counterpart: https://github.com/arcboxlabs/linkcodehq/pull/40