fix(effect): restore HTTPS bind on 0.0.0.0 after Phase 8 daemon migration#3
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the daemon’s server binding/config threading after the Phase 8 migration by making the live DaemonConfigRef the single source of truth for host/port/TLS state, ensuring HTTPS binds correctly (e.g., 0.0.0.0) and that the actual ephemeral port is propagated consistently across status/setup-info/persistence.
Changes:
- Refactors server startup to take explicit bind/TLS config and to write the actual bound port back into
DaemonConfigRef. - Reworks
SetupInfoProviderto use getter functions so/api/setup-inforeflects live port/TLS changes. - Adds/updates unit tests covering binding behavior, onboarding setup-info composition, and daemon status/persistence after runtime config updates.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/server/http-server-layer.test.ts | Updates SetupInfoProvider test wiring to use live getters and adds coverage for dynamic setup-info values. |
| test/unit/effect/layer-wiring.test.ts | Updates daemon layer wiring tests to seed full initial config and assert ref reflects actual ephemeral port. |
| test/unit/effect/http-server-live.test.ts | Adds integration-style layer tests for HTTP/onboarding servers, TLS material usage, and config ref port handoff. |
| test/unit/effect/daemon-main-getstatus.test.ts | Adds daemon-main status/persistence tests covering TLS host/port reporting and restart config application. |
| test/unit/effect/daemon-config-ref.test.ts | Extends config-ref tests to cover tlsEnabled/hostExplicit propagation. |
| test/unit/daemon/daemon-onboarding.test.ts | Updates onboarding lifecycle tests for the new explicit onboarding start config. |
| test/unit/daemon/daemon-lifecycle-ipc.test.ts | Adjusts lifecycle IPC tests for removal of host/port from lifecycle context. |
| test/unit/daemon/daemon-lifecycle-bind.test.ts | Adds lifecycle bind tests verifying startHttpServer returns actual port and redirects don’t use :0. |
| test/fixtures/test-key.pem | Adds TLS key fixture used by new TLS-focused tests. |
| test/fixtures/test-cert.pem | Adds TLS cert fixture used by new TLS-focused tests. |
| src/lib/server/effect-http-router.ts | Changes SetupInfoProvider contract to getters and uses live values when composing URLs. |
| src/lib/relay/relay-stack.ts | Updates relay stack to provide SetupInfoProvider via getters. |
| src/lib/effect/daemon-main.ts | Threads full runtime config into DaemonConfigRef, snapshots/updates runtime config, and removes reliance on mutable ctx bind fields. |
| src/lib/effect/daemon-layers.ts | Makes HTTP/onboarding server layers read bind/TLS from tags, start with explicit configs, and write back actual port. |
| src/lib/effect/daemon-config-ref.ts | Extends config creation to accept explicit hostExplicit. |
| src/lib/daemon/daemon-lifecycle.ts | Refactors startHttpServer/startOnboardingServer to accept explicit start configs and returns actual bound port; improves close behavior. |
| pnpm-workspace.yaml | Updates pnpm build allowlist/denylist configuration. |
| package.json | Adjusts pnpm built-dependency allow/ignore configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DaemonConfigRef, including bind settings, TLS state, keep-awake config, dismissed paths, and persisted session counts.DaemonLifecycleContexta server-handle sink only;host/port/tlsare removed from its interface.startHttpServer/startOnboardingServeraccept explicit config params instead of reading mutable ctx fields.makeHttpServerLive/makeOnboardingServerLiveread host/port fromDaemonConfigRefTagand TLS/CA material fromTlsCertTag.Root cause
Phase 8 (
b3d0f8b) introducedDaemonConfigRefTagbut left the server bind path split between the Ref and stale mutableDaemonLifecycleContextfields.tlsEnabledandhostExplicitwere not threaded into the initial Ref, so cert loading could short-circuit; server startup then read the pre-TLS ctx snapshot and bound to the wrong interface.Test plan
pnpm checkpnpm lintpnpm test:unitpnpm test:allRelay: https://0.0.0.0:2633.lsof -nP -i :2633shows*:2633, not127.0.0.1:2633.curl -k https://$TS_IP:2633/api/statusreturns JSON withtlsEnabled: trueandhost: "0.0.0.0".http://$TS_IP:2633/redirects tohttps://$TS_IP:2633/.$TS_IP:2634serves/api/setup-infowith correct URLs and/ca/downloadreturns cert content.