From 5edd852cfa05d9da2b6bbaa1b7c3a8fa8afbd8aa Mon Sep 17 00:00:00 2001 From: Sid Date: Sat, 2 May 2026 20:56:06 +0800 Subject: [PATCH] =?UTF-8?q?fix(daemon):=20restore=20startServer=20Promise?= =?UTF-8?q?=20contract=20=E2=80=94=20return=20url=20/=20{=20url,=20server?= =?UTF-8?q?=20}=20(#268)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(daemon): restore startServer Promise contract — return url / { url, server } PR #258 ("Standardize agent communication via stdin and remove Windows-specific shims logic") removed the Promise wrap around `startServer`'s `app.listen()` call. Two regressions shipped to main as a result: 1. **`returnServer: true` returns just `server`, not `{ url, server }`.** Both `apps/daemon/sidecar/server.ts:65` and `apps/daemon/tests/version-route.test.ts:10` cast the return value to `{ url, server }` and read `started.url`. Post-#258 those reads land on `undefined`, which becomes `"undefined/api/version"` URLs at runtime — the sidecar can no longer report its bound port to the parent process, and `version-route.test.ts` fails with `ERR_INVALID_URL`. 2. **`returnServer: false` returns nothing.** `apps/daemon/src/cli.ts:77` calls `startServer({ port }).then(url => console.log(\`[od] listening on \${url}\`))` — post-#258 that prints `[od] listening on undefined`, and the subsequent `spawn(opener, [url], …)` to open the browser also gets `undefined`. The daemon still binds, but the two paths that *consume* the URL are silently broken. Both stem from the same change: the new code synchronously kicks off `app.listen` and returns either the server object or `undefined` before `listen`'s callback fires, so neither the resolved port nor the URL string ever reach the caller. This restores the pre-#258 Promise wrap, which already had the right shape for all three call sites: ```ts return await new Promise((resolve) => { const server = app.listen(port, () => { const url = `http://127.0.0.1:${server.address().port}`; resolve(returnServer ? { url, server } : url); }); }); ``` Behavior contract restored: | `returnServer` | resolves with | |---|---| | `false` (default) | `string` — `http://127.0.0.1:` | | `true` | `{ url: string, server: http.Server }` | The Promise resolution waits for `listen`'s callback so `port=0` (ephemeral) callers always see the actual bound port, never the placeholder. Test coverage: `tests/version-route.test.ts` already exercises the `{ url, server }` path via `await fetch(\`\${baseUrl}/api/version\`)`. After this fix the suite is back to 15 files / 225 tests green (2 of those — both in `version-route` — failed against current main). Verification: - `pnpm --filter @open-design/daemon test` — 15 files / 225 tests pass - `tsc -p apps/daemon/tsconfig.json --noEmit` — clean - `tsc -p apps/daemon/tsconfig.tests.json --noEmit` — clean (was failing on the `as { url, server }` cast before) * fix(daemon): reject startServer Promise on bind failure / null address Folds in the two non-blocking improvements @lefarcen flagged on the review of the parent commit: 1. **Explicit `error` event listener.** `app.listen` throws synchronously for some failure modes (e.g. invalid port) and emits an `error` event for others (EADDRINUSE on certain Node versions, EACCES, EADDRNOTAVAIL). Without an `error` listener the returned Promise would hang forever on the event-emitting paths instead of surfacing the failure to the caller. 2. **Null-`address()` guard.** `server.address()` typing allows `string | AddressInfo | null`. For TCP listeners it's always AddressInfo, but if a future code path ever returns null (or a Unix socket path string), the previous fallback `port` would still be `0` for ephemeral-port callers and quietly produce a `http://127.0.0.1:0` URL that fetches would fail on. Reject the Promise instead with an explicit diagnostic. Both keep the success path identical — `version-route.test.ts` still exercises the same `{ url, server }` contract on `port: 0` and passes unchanged. 15/15 daemon test files / 225/225 tests pass. Refs review feedback on #268 (P2 suggestions, non-blocking). * fix(daemon): drop a93246d workaround for restored startServer contract After rebasing onto current `main`, this PR's restored `{ url, server }` return shape from `startServer` collides with the workaround applied in `a93246d` (PR #271), which had patched the two consumers — `apps/daemon/sidecar/server.ts` and `apps/daemon/tests/version-route.test.ts` — to call `started.address()` on a bare `Server` handle. Revert both consumers to the pre-`a93246d` `{ url, server }` shape so they line up with the contract restored in `5c2f520`. With this: - `pnpm --filter @open-design/daemon test` — 246/246 pass - `pnpm --filter @open-design/daemon build` — tsc clean for both tsconfig.json and tsconfig.sidecar.json - `apps/daemon/src/cli.ts` (which expects a URL string and was printing "listening on undefined" on `main`) is now correct without any further change. * fix(i18n): add missing chat.comments.* keys to hu.ts PR #288 added Hungarian without the 12 `chat.comments.*` keys that PR #284 introduced — the two landed in parallel and only en + the older locales got both. Without these, `pnpm --filter @open-design/web typecheck` errors on `src/i18n/locales/hu.ts(3,14): TS2740 ... missing the following properties from type 'Dict': 'chat.comments.attached', 'chat.comments.emptyAttached', 'chat.comments.saved', and 9 more`, which blocks every PR's CI. Drive-by unblock with English fallback strings — proper Hungarian translations should follow in a separate i18n pass. --------- Co-authored-by: lefarcen <935902669@qq.com> --- apps/daemon/sidecar/server.ts | 18 +++++------ apps/daemon/src/server.ts | 40 +++++++++++++++++++------ apps/daemon/tests/version-route.test.ts | 16 ++++------ apps/web/src/i18n/locales/hu.ts | 12 ++++++++ 4 files changed, 56 insertions(+), 30 deletions(-) diff --git a/apps/daemon/sidecar/server.ts b/apps/daemon/sidecar/server.ts index 977a9fe13..36067a677 100644 --- a/apps/daemon/sidecar/server.ts +++ b/apps/daemon/sidecar/server.ts @@ -62,23 +62,19 @@ function attachParentMonitor(stop: () => Promise): void { } export async function startDaemonSidecar(runtime: SidecarRuntimeContext): Promise { - const started = await startServer({ port: parsePort(process.env[DAEMON_PORT_ENV]), returnServer: true }) as unknown as - | Server - | undefined; - if (started == null) { + const started = await startServer({ port: parsePort(process.env[DAEMON_PORT_ENV]), returnServer: true }) as + | string + | { server: Server; url: string }; + if (typeof started === "string") { throw new Error("daemon startServer did not return a server handle"); } - const server = started; - const address = server.address(); - if (address == null || typeof address === "string") { - throw new Error("daemon startServer did not bind to a TCP port"); - } + const serverHandle = started; const state: DaemonStatusSnapshot = { pid: process.pid, state: "running", updatedAt: new Date().toISOString(), - url: `http://127.0.0.1:${address.port}`, + url: serverHandle.url, }; let ipcServer: JsonIpcServerHandle | null = null; let stopped = false; @@ -93,7 +89,7 @@ export async function startDaemonSidecar(runtime: SidecarRuntimeContext undefined); - await closeHttpServer(server).catch(() => undefined); + await closeHttpServer(serverHandle.server).catch(() => undefined); resolveStopped(); } diff --git a/apps/daemon/src/server.ts b/apps/daemon/src/server.ts index 4b7a5f196..c90d62b5d 100644 --- a/apps/daemon/src/server.ts +++ b/apps/daemon/src/server.ts @@ -2277,16 +2277,38 @@ export async function startServer({ port = 7456, returnServer = false } = {}) { } }); - const server = app.listen(port, () => { - resolvedPort = server.address().port; - if (!returnServer) { - console.log(`[od] daemon listening on http://127.0.0.1:${resolvedPort}`); - } + // Wait for `listen` to bind so callers always see the resolved URL — + // critical when port=0 (ephemeral port) and when the embedding sidecar + // needs to advertise the port to a parent process before any request + // can flow. Three callers depend on this contract: + // - `apps/daemon/src/cli.ts` → expects a `url` string + // - `apps/daemon/sidecar/server.ts` → expects `{ url, server }` + // - `apps/daemon/tests/version-route.test.ts` → expects `{ url, server }` + return await new Promise((resolve, reject) => { + const server = app.listen(port, () => { + const address = server.address(); + // `address()` can in theory return `string | AddressInfo | null`. For + // a TCP listener it's always `AddressInfo` with a `.port` — the guard + // is belt-and-braces so an unexpected null never silently produces a + // `http://127.0.0.1:0` URL that callers would then try to fetch. + const boundPort = address && typeof address === 'object' ? address.port : null; + if (!boundPort) { + reject(new Error(`[od] daemon failed to resolve listening port (address=${JSON.stringify(address)})`)); + return; + } + resolvedPort = boundPort; + const url = `http://127.0.0.1:${resolvedPort}`; + if (!returnServer) { + console.log(`[od] daemon listening on ${url}`); + } + resolve(returnServer ? { url, server } : url); + }); + // `app.listen` throws synchronously when the port is already in use on + // some Node versions, but emits an `error` event on others (and for + // EACCES / EADDRNOTAVAIL even on the same Node). Wire the event so the + // returned Promise always settles instead of hanging forever. + server.on('error', reject); }); - - if (returnServer) { - return server; - } } function randomId() { diff --git a/apps/daemon/tests/version-route.test.ts b/apps/daemon/tests/version-route.test.ts index 0f469b1c5..ecae6775f 100644 --- a/apps/daemon/tests/version-route.test.ts +++ b/apps/daemon/tests/version-route.test.ts @@ -7,16 +7,12 @@ describe('/api/version', () => { let baseUrl: string; beforeAll(async () => { - const started = await startServer({ port: 0, returnServer: true }) as http.Server | undefined; - if (started == null) { - throw new Error('startServer did not return a server handle'); - } - const address = started.address(); - if (address == null || typeof address === 'string') { - throw new Error('startServer did not bind to a TCP port'); - } - server = started; - baseUrl = `http://127.0.0.1:${address.port}`; + const started = await startServer({ port: 0, returnServer: true }) as { + url: string; + server: http.Server; + }; + baseUrl = started.url; + server = started.server; }); afterAll(() => new Promise((resolve) => server.close(() => resolve()))); diff --git a/apps/web/src/i18n/locales/hu.ts b/apps/web/src/i18n/locales/hu.ts index 11916bb6e..94cf0ed94 100644 --- a/apps/web/src/i18n/locales/hu.ts +++ b/apps/web/src/i18n/locales/hu.ts @@ -334,6 +334,18 @@ export const hu: Dict = { 'chat.tabChat': 'Csevegés', 'chat.tabComments': 'Megjegyzések', 'chat.commentsSoon': 'Megjegyzések — hamarosan', + 'chat.comments.attached': 'Attached to chat', + 'chat.comments.emptyAttached': 'No comments attached.', + 'chat.comments.saved': 'Saved comments', + 'chat.comments.emptySaved': 'No saved comments.', + 'chat.comments.add': 'Add', + 'chat.comments.addAll': 'Add all', + 'chat.comments.remove': 'Remove', + 'chat.comments.placeholder': 'Comment on this element…', + 'chat.comments.addSend': 'Add & send', + 'chat.comments.updateSend': 'Update & send', + 'chat.comments.removeAttachment': 'Remove comment attachment', + 'chat.comments.removeAttachmentAria': 'Remove comment attachment for {name}', 'chat.conversationsTitle': 'Beszélgetések', 'chat.conversationsAria': 'Beszélgetések előzménye', 'chat.newConversation': 'Új beszélgetés',