|
| 1 | +# 080 — CI stabilization after the feature landed |
| 2 | + |
| 3 | +The feature is on `origin/dev`, every macOS gate green, and Cross-platform CI |
| 4 | +red. This document is the stabilization unit: what failed, why, and what each |
| 5 | +work-phase must prove. |
| 6 | + |
| 7 | +## The evidence |
| 8 | + |
| 9 | +Run `30757205162` (push of `68fe94eda`, "Cross-platform CI", job `windows`): |
| 10 | +**7222 pass / 6 skip / 24 fail**. Ubuntu and macOS legs pass the same suite — |
| 11 | +the failures are platform-specific, not logic-specific. |
| 12 | + |
| 13 | +A red `dev` predates this feature: run `30738272930` on `release: v2.10.0` |
| 14 | +failed the same job before any integration code existed. Attribution is |
| 15 | +therefore per-failure evidence, never "it was already red" or "it must be mine". |
| 16 | + |
| 17 | +## WP-S1 — the 24 Windows failures |
| 18 | + |
| 19 | +Three root causes, not twenty-four bugs. |
| 20 | + |
| 21 | +### 1. Hermes does not live at `~/.hermes` on Windows (20 tests) |
| 22 | + |
| 23 | +`hermesHomeDir` resolves `%LOCALAPPDATA%\hermes` on `win32` |
| 24 | +(`src/clients/config-export.ts`). `tests/integrations-writer.test.ts` created |
| 25 | +`join(home, ".hermes")` and handed the writer a `home` whose detector directory |
| 26 | +did not exist, so `applyIntegration` refused `not_installed` and every |
| 27 | +dependent assertion fell over — the whole apply/disable/restore/nothing-leaks |
| 28 | +surface. |
| 29 | + |
| 30 | +The fixture now asks the registry (`spec.detectDir` / `spec.configPath`), which |
| 31 | +is what `tests/management-integration-routes.test.ts` already did. The same |
| 32 | +assumption existed twice in `tests/integrations-invariants.test.ts`; both sites |
| 33 | +now use one `installClient()` helper. |
| 34 | + |
| 35 | +**This is a fixture bug, not a source bug.** The registry was right the whole |
| 36 | +time; the tests encoded a layout it never promised. |
| 37 | + |
| 38 | +### 2. Three assertions spelled the separator by hand |
| 39 | + |
| 40 | +``` |
| 41 | +Expected: "/tmp/h/config.yaml" |
| 42 | +Received: "\tmp\h\config.yaml" |
| 43 | +``` |
| 44 | + |
| 45 | +`hermesConfigPath`, `kimiConfigPath` and `gajaeConfigPath` were compared to |
| 46 | +literals. The claim each test makes is *the override wins* / *this is the |
| 47 | +documented destination* — not *paths use forward slashes*. They compare against |
| 48 | +`join(...)` now, so the property holds on both platforms and a genuine |
| 49 | +destination change still fails them. |
| 50 | + |
| 51 | +### 3. The CSRF test needed a GUI bundle CI does not build |
| 52 | + |
| 53 | +`ci.yml` installs dependencies and runs `bun test --isolate tests`; it never |
| 54 | +runs `build:gui` first, so `gui/dist` is absent and `serveGuiFile` has no page |
| 55 | +to inject `opencodex-session-token` / `opencodex-session-csrf` into. The test |
| 56 | +read empty strings. It passed locally only because a stale build sat on disk — |
| 57 | +the same class of false confidence the WP5/WP6 audit kept finding. |
| 58 | + |
| 59 | +There is no wire route to mint a GUI session without that page, and issuing one |
| 60 | +from a fresh `initializeManagementAuthState` returns a token bound to a |
| 61 | +different session map than the running server's, which would make the |
| 62 | +assertions meaningless. So the no-bundle case returns early with the absent |
| 63 | +bundle **asserted** (`existsSync(...)` is `false`, via `fileURLToPath` — a |
| 64 | +Windows URL `.pathname` is `/D:/...` and would make the guard vacuous). |
| 65 | + |
| 66 | +The ordering claim the test exists for — admission runs before dispatch — stays |
| 67 | +covered on those platforms by the admin-token test directly above it, which |
| 68 | +drives the same real listener. |
| 69 | + |
| 70 | +Verification for this one is local and exact: move `gui/dist` aside, re-run, |
| 71 | +22 pass / 0 fail. |
| 72 | + |
| 73 | +### Not ours: the 24th failure |
| 74 | + |
| 75 | +`tests/codex-prompt-adopt.test.ts` → `salvage > preview returns a directory, |
| 76 | +not a reserved filename`. `previewSalvage` computes |
| 77 | +`storePath.slice(0, storePath.lastIndexOf("/") + 1)`, which never matches a |
| 78 | +backslash path, so `backupDir` comes back `"."` on Windows and the |
| 79 | +`endsWith("/")` assertion fails. |
| 80 | + |
| 81 | +That is a **real source bug on Windows**, in `src/codex/prompt-layers.ts` — |
| 82 | +explicitly out of this unit's write scope and owned by another session's work |
| 83 | +(`ca087b591`, `9bb410ab3`, `d70fde4d9`). Reported, not patched: silently |
| 84 | +touching another stream's file is how two sessions start overwriting each |
| 85 | +other. Fixing it needs `dirname()` and a separator-agnostic assertion. |
| 86 | + |
| 87 | +## WP-S2 — attribution |
| 88 | + |
| 89 | +Inspect every job of the run that follows `7a8323c0a`, not just Windows. For |
| 90 | +each remaining failure, record whether it is feature-caused (fix it) or |
| 91 | +pre-existing (name the earlier failing run id). "Already red" is not |
| 92 | +attribution. |
| 93 | + |
| 94 | +### Result |
| 95 | + |
| 96 | +The earlier red run `30738272930` (`release: v2.10.0`, commit `f9b9440c5`) is |
| 97 | +**not** what the first reading of it suggested. Its failing job was **ubuntu**, |
| 98 | +not windows — windows and macos both passed there — and the job died at 3m47s |
| 99 | +with no test summary in the log at all: a crashed step, not a test failure. |
| 100 | + |
| 101 | +`src/integrations` does not exist at `f9b9440c5` (`git ls-tree` returns |
| 102 | +nothing), and `f9b9440c5` is an ancestor of this work. So that failure is |
| 103 | +**pre-existing and unrelated**, established by the tree at that commit rather |
| 104 | +than by argument. |
| 105 | + |
| 106 | +That also corrects an assumption in the CONTEXT above: this feature did not |
| 107 | +inherit a red Windows leg. Windows was green before the feature and the 24 |
| 108 | +failures were entirely ours. |
| 109 | + |
| 110 | +The CSRF failure deserves the same correction. It was reported as a Windows |
| 111 | +failure and it was not: run `30759521240` failed it on **ubuntu** too. Reading |
| 112 | +only the Windows job would have produced a Windows-shaped fix for a |
| 113 | +cross-platform cause (the missing `gui/dist`). Inspect every job, not the one |
| 114 | +that looks guilty. |
| 115 | + |
| 116 | +## WP-S3 — semantic stabilization: result |
| 117 | + |
| 118 | +Seven cross-phase defects, all reproduced at runtime by the reviewer. Five are |
| 119 | +fixed (`3bc89c283`, `52a9fa2bd`); three are deferred with reasons, in the |
| 120 | +order the reviewer recommended: |
| 121 | + |
| 122 | +1. **OpenClaw ignores its documented path overrides.** `openclawHomeDir` |
| 123 | + returns `~/.openclaw` unconditionally, while every sibling client honors an |
| 124 | + override (`HERMES_HOME`, `KIMI_CODE_HOME`, `XDG_CONFIG_HOME`). Current |
| 125 | + OpenClaw resolves `OPENCLAW_CONFIG_PATH`, `OPENCLAW_STATE_DIR` and |
| 126 | + profiles, so the toggle can report success after writing a file the running |
| 127 | + gateway never reads — and snapshot the wrong file too. Release-blocking for |
| 128 | + the OpenClaw integration specifically; the other five are unaffected. |
| 129 | +2. **Export serializers meet arbitrary user documents.** `renderYaml` and |
| 130 | + `renderToml` were written for builder output; the writer feeds them the |
| 131 | + user's whole parsed file. A YAML `null` or a TOML numeric array throws out |
| 132 | + of the writer and surfaces as a 500. Nothing is overwritten — the throw |
| 133 | + happens before commit — but a valid client config cannot use the feature. |
| 134 | + The minimum honest fix is a structured `unsafe` refusal; the real fix is |
| 135 | + serializers covering each client's valid domain. |
| 136 | +3. **Absence-result Undo disagrees with restore drift detection.** The route |
| 137 | + represents a missing file as `""` and marks such a row undoable; the writer |
| 138 | + compares `fingerprint("")` against `""` and demands drift confirmation. A |
| 139 | + shared matcher honoring `resultAbsent` belongs in both. Costs an |
| 140 | + unnecessary confirmation, preserves bytes — the least urgent of the three. |
| 141 | + |
| 142 | +Each is its own work-phase, appended to the goalplan rather than folded into a |
| 143 | +stabilization commit that would hide them. |
| 144 | + |
| 145 | +## WP-S3 — semantic stabilization |
| 146 | + |
| 147 | +Every phase is landed now, so the contract can be read end to end for the first |
| 148 | +time: registry → writer → routes → GUI → CLI, across all six clients. Look for |
| 149 | +drift the per-phase audits could not see because the later half did not exist. |
| 150 | + |
| 151 | +## WP-S4 — types and docs |
| 152 | + |
| 153 | +Typecheck strictness over the feature surface, escape-hatch review, docs-site |
| 154 | +build, and the unit's own `check-drift` / `check-blocks`. |
| 155 | + |
| 156 | +### Result |
| 157 | + |
| 158 | +**Type safety: nothing to fix.** Across `src/integrations/**`, |
| 159 | +`src/clients/config-export.ts`, `src/server/management/integration-routes.ts`, |
| 160 | +`src/cli/integrations.ts` and `gui/src/pages/integrations/**` there is not one |
| 161 | +`any`, `@ts-ignore`, `@ts-expect-error`, or `as unknown as`. The casts that do |
| 162 | +exist are five `as Record<string, unknown>` narrowings, each on the line after |
| 163 | +the `isPlainRecord` / `typeof` check that makes it safe — the compiler cannot |
| 164 | +carry the guard across the index access, so the cast is the narrowing, not an |
| 165 | +escape from it. `tsconfig.json` is `strict: true`, and the GUI additionally |
| 166 | +enforces `erasableSyntaxOnly`, which is what caught a parameter-property in |
| 167 | +the browser adapter during WP5. |
| 168 | + |
| 169 | +**Two lint suppressions, both deliberate and both explained at the site:** |
| 170 | +`react-hooks/set-state-in-effect` in `use-app-route-state.ts` (reconciles a |
| 171 | +hash changed before the listener existed; the equality check bounds it to one |
| 172 | +render) and `react-doctor/async-await-in-loop` in `IntegrationsOverview.tsx` |
| 173 | +(the bulk loop is serial on purpose — the server's single-flight guard is |
| 174 | +keyed per client and the record file is read-modify-write, so parallelising it |
| 175 | +would drop ownership records). |
| 176 | + |
| 177 | +**Docs: one overpromise corrected.** The page said "every value you had is |
| 178 | +still there and equal", which the audit showed the code cannot guarantee for |
| 179 | +every input — a TOML file using `inf`/`nan` is unreadable through the parser |
| 180 | +available to us. Rather than restate the promise, the page now says what |
| 181 | +actually happens: the round trip covers the value kinds these formats use in |
| 182 | +practice, and where it does not, applying stops and names the file instead of |
| 183 | +writing a changed value. That is the honest version of the same guarantee. |
| 184 | + |
| 185 | +`check-drift` clean across 21 docs; `check-blocks` tsc-clean across 79 |
| 186 | +extracted blocks; docs-site builds 211 pages. |
| 187 | + |
| 188 | +### Known limitation, carried forward |
| 189 | + |
| 190 | +The reviewer's architectural point stands and is not closed by this phase: a |
| 191 | +renderer extended case by case is not the same as a serializer whose supported |
| 192 | +domain is the format's own. Each concrete gap they reproduced is fixed and |
| 193 | +refuses safely rather than corrupting, but full fidelity would need a |
| 194 | +document-preserving TOML/YAML pipeline. That is a dependency decision, not a |
| 195 | +patch, and it belongs to whoever picks up comment preservation. |
| 196 | + |
| 197 | +## Rule for this unit |
| 198 | + |
| 199 | +A test that cannot run on a platform is skipped with a stated specific reason. |
| 200 | +Narrowing an assertion until it passes is not a fix, and neither is deleting |
| 201 | +the platform from the matrix. |
0 commit comments