修復 LAN WebRTC 多人觀看與 Kit build preflight - #130
Conversation
Add LAN/public runtime handoff parameters, default spectator endpoint generation, and deploy-time Kit build artifact preflight. Phase 2 now runs bim-streaming-server repo.bat build when host-native Kit runtime artifacts are missing, failing early with a persisted build log instead of timing out in Phase 4b.
|
Warning Review limit reached
More reviews will be available in 21 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR refactors the hybrid Docker web-plane + host-native Kit demo to support configurable LAN exposure and spectator WebRTC endpoint generation. It introduces PUBLIC_HOST/VIEWER_BIND_HOST environment variables, auto-generates up to 5 spectator endpoints in the coordinator from port configuration, extends deploy.ps1 to resolve and wire public host/spectator ports, adds kit runtime artifact auto-build in Phase 2, and includes comprehensive OpenSpec design/specification/test coverage. ChangesLAN Public Host & Spectator Endpoint Generation
Deployment Orchestration & Kit Runtime Auto-Fix
Test Coverage & OpenSpec Specifications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements the OpenSpec change fix-lan-runtime-params-spectator-capacity by making the hybrid (Docker web plane + host-native Kit) deployment LAN-friendly (public host/base URLs instead of loopback), adding same-Kit primary + spectator WebRTC endpoint topology, and adding a Kit runtime build-artifact preflight/auto-build step to avoid late timeouts.
Changes:
- Add coordinator config support (and tests) for generating spectator Kit endpoints from a single primary endpoint, while preserving explicit
KIT_INSTANCE_ENDPOINTSoverrides. - Extend hybrid deployment scripts/compose/env examples to carry
PUBLIC_HOST, public coordinator/viewer bases, viewer bind host, and spectator port topology. - Add host-native preflight + deploy auto-fix for missing Kit runtime build artifacts (run
bim-streaming-server\repo.bat buildearly, log toscripts\.run\kit-repo-build.log).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/tests/test-preflight-ports.ps1 | Adds test ensuring spectator ports are included in host-native port audits. |
| scripts/tests/test-preflight-host-native.ps1 | Adds tests for kit runtime artifact OK/NEEDS_BUILD preflight outcomes. |
| scripts/tests/test-host-native-launcher.ps1 | Adds assertions that launcher forwards spectator port args. |
| scripts/start-web-plane-docker.ps1 | Prints coordinator/viewer public URLs (and viewer bind host) for hybrid mode. |
| scripts/lib/preflight-ports.ps1 | Allows adding extra host-native ports (spectator ports) into port availability checks. |
| scripts/lib/preflight-host-native.ps1 | Adds _build artifact detection for Kit runtime and exposes build hints in audit output. |
| scripts/lib/host-native-launcher.ps1 | Adds bind host + public artifacts URL for conversion; forwards spectator ports to Kit launcher. |
| scripts/deploy.ps1 | Adds LAN/public host + spectator topology params; sets env wiring; auto-builds Kit artifacts when missing. |
| scripts/check-web-plane-docker.ps1 | Surfaces coordinator/viewer public URLs in hybrid diagnostics output. |
| openspec/changes/fix-lan-runtime-params-spectator-capacity/tasks.md | Tracks implementation tasks and validation steps for the change. |
| openspec/changes/fix-lan-runtime-params-spectator-capacity/specs/runtime-verification-task-status/spec.md | Updates runtime verification requirements for same-Kit primary/spectator evidence. |
| openspec/changes/fix-lan-runtime-params-spectator-capacity/specs/multi-artifact-kit-routing/spec.md | Clarifies single-Kit multi-viewer vs dedicated multi-Kit routing evidence separation. |
| openspec/changes/fix-lan-runtime-params-spectator-capacity/specs/local-coordinator-ifc-ready-intake-boundary/spec.md | Defines /ui/open LAN-safe redirect behavior and trusted base URL handling requirements. |
| openspec/changes/fix-lan-runtime-params-spectator-capacity/specs/docker-web-plane-host-native-kit/spec.md | Defines hybrid deployment requirements, LAN bind host, and Kit artifact preflight/build behavior. |
| openspec/changes/fix-lan-runtime-params-spectator-capacity/proposal.md | Documents problem statement, goals, non-goals, and success criteria. |
| openspec/changes/fix-lan-runtime-params-spectator-capacity/design.md | Documents configuration model, topology generation, and validation strategy. |
| openspec/changes/fix-lan-runtime-params-spectator-capacity/.openspec.yaml | Adds OpenSpec metadata for the change. |
| docs/verification/2026-05-27-fix-lan-runtime-params-spectator-capacity.md | Records pre-PR verification commands/results and GitNexus limitations evidence. |
| compose.host-kit.yml | Wires PUBLIC_HOST, public base URLs, viewer bind host, and spectator env vars into hybrid compose. |
| bim-review-coordinator/tests/config.test.ts | Adds unit tests for spectator endpoint generation and explicit endpoint override behavior. |
| bim-review-coordinator/src/config.ts | Implements spectator endpoint generation from env-driven count/start/stride with validation scaffolding. |
| .env.web-plane.host-kit.example | Adds documented LAN/public host parameters and default spectator topology settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ($trimmed -match '^https?://') { | ||
| return ([uri]$trimmed).Host | ||
| } |
| $script:coordinatorPublicUrl = "http://${resolvedPublicHost}:$resolvedCoordinatorPort" | ||
| $script:viewerPublicUrl = "http://${resolvedPublicHost}:$resolvedViewerPort" |
| const mediaPort = mediaStart + index * stride; | ||
| if (signalingPort > 65535 || mediaPort > 65535) { | ||
| throw new Error("KIT_SPECTATOR_* generated a port outside 1-65535."); | ||
| } |
| expect(config.kitInstanceEndpoints.slice(1).map((endpoint) => endpoint.mediaPort)) | ||
| .toEqual([48008, 48018, 48028, 48038, 48048]); | ||
| }); | ||
|
|
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 556cc70af3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $resolvedSpectatorSignalPorts = @(New-PortSequence -Count $resolvedSpectatorCount -Start $resolvedSpectatorSignalStart -Stride $resolvedSpectatorStride -Name 'KIT_SPECTATOR_SIGNALING_PORT') | ||
| $resolvedSpectatorMediaPorts = @(New-PortSequence -Count $resolvedSpectatorCount -Start $resolvedSpectatorMediaStart -Stride $resolvedSpectatorStride -Name 'KIT_SPECTATOR_MEDIA_PORT') |
There was a problem hiding this comment.
When an operator overrides KIT_SPECTATOR_SIGNALING_PORT_START or KIT_SPECTATOR_MEDIA_PORT_START to the primary values (for example 49100 or 47998), this only checks the generated spectator sequence against itself, not against $KitSignalPort/$KitMediaPort. I checked bim-streaming-server/scripts/start-streaming-server.ps1; it calls Test-PortFree for all ports before Kit binds, so a duplicate primary/spectator port is not rejected there either, and the coordinator/deploy path can advertise/start two streams on the same port. Please reject overlaps with the primary Kit ports before preflight/launch.
Useful? React with 👍 / 👎.
| $startInfo = Start-HostNativeKit ` | ||
| -RepoRoot $RepoRoot ` | ||
| -SignalPort $KitSignalPort ` | ||
| -StreamPort $KitMediaPort ` | ||
| -PublicIp $resolvedPublicHost ` | ||
| -SpectatorSignalPorts $resolvedSpectatorSignalPorts ` | ||
| -SpectatorStreamPorts $resolvedSpectatorMediaPorts |
There was a problem hiding this comment.
If scripts\.run\bim-streaming-server.pid already exists, Phase 4b skips Kit entirely, so these new launch arguments are never applied on a rerun. A common path is running the old local deployment, then rerunning deploy with -PublicHost or a different spectator count: the web plane can be refreshed, but the existing Kit process still has the old publicIp and no/new wrong spectator ports, while the coordinator advertises the updated endpoints. Please compare the desired Kit runtime parameters with the running instance or force a restart when they change.
Useful? React with 👍 / 👎.
| if ($SpectatorSignalPorts.Count -gt 0) { | ||
| $arguments += '-SpectatorSignalPorts' | ||
| $arguments += ($SpectatorSignalPorts -join ',') | ||
| $arguments += '-SpectatorStreamPorts' | ||
| $arguments += ($SpectatorStreamPorts -join ',') |
There was a problem hiding this comment.
When -PublicHost is set for a LAN demo, this now adds spectator ports but still only passes one -PublicIp value, and bim-streaming-server/scripts/start-streaming-server.ps1 applies that setting only to primaryStream/publicIp. NVIDIA's omni.kit.livestream.app docs state spectator streams support the same settings as primary except dynamic resize, including publicIp (https://docs.omniverse.nvidia.com/kit/docs/omni.kit.livestream.app/latest/Overview.html), so remote spectators can receive unroutable ICE/media addresses even though the coordinator advertises their LAN endpoints. Please forward/apply publicIp for each generated spectator stream as well.
Useful? React with 👍 / 👎.
|
|
||
| $dockerPorts = @(8004, 5173) | ||
| $hostNativePorts = @(49100, 49101, 47998) | ||
| $hostNativePorts = @(@(49100, 49101, 47998) + $ExtraHostNativePorts | Sort-Object -Unique) |
There was a problem hiding this comment.
The new ExtraHostNativePorts list includes spectator media ports such as 48008, but the lookup above this line only calls Get-NetTCPConnection. WebRTC streamPort is UDP (also documented for omni.kit.livestream.app), so a process already bound to UDP 48008/48018 is reported as FREE, Phase 3 never prompts, and Kit can fail only after startup. Please check UDP listeners (for media ports) in addition to TCP signaling listeners when auditing the generated spectator ports.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bim-review-coordinator/src/config.ts`:
- Around line 206-218: The function kitInstanceEndpointsFromEnv currently
silences malformed KIT_INSTANCE_ENDPOINTS and falls back; instead, fail fast: if
process.env[name] exists but JSON.parse throws or the parsed value is not an
array, throw a descriptive Error (include the env var name and the raw value and
parsing error if available) rather than returning
withGeneratedSpectatorEndpoints; likewise, after mapping via endpointFromUnknown
if the resulting endpoints array is empty (endpoints.length === 0) throw an
Error indicating no valid endpoints were produced from the env value; update
kitInstanceEndpointsFromEnv to propagate these errors so callers can fail fast.
In `@bim-review-coordinator/tests/config.test.ts`:
- Around line 203-221: The test relies on specific KIT_* env vars but doesn't
clear preexisting environment state; before calling loadConfig() in the
"generates configured spectator endpoints from a single primary endpoint" (and
the other similar test), explicitly remove any existing KIT_INSTANCE_ENDPOINTS
and any KIT_SPECTATOR_* env variables (e.g., delete
process.env.KIT_INSTANCE_ENDPOINTS and iterate keys to delete process.env
entries that startWith("KIT_SPECTATOR_")), then set the deterministic vars
(KIT_STREAM_SERVER, KIT_MEDIA_SERVER, KIT_MEDIA_PORT, KIT_SPECTATOR_COUNT) and
call loadConfig(); this ensures loadConfig() runs with a clean, deterministic
environment and prevents CI/dev-shell bleed causing flakey assertions.
In `@scripts/deploy.ps1`:
- Around line 253-255: After generating $resolvedSpectatorSignalPorts and
$resolvedSpectatorMediaPorts with New-PortSequence, add explicit collision
checks to ensure no spectator port equals the primary ports ($KitSignalPort,
$KitMediaPort) and that the two spectator sets do not intersect; if any
intersection is found, either adjust/regen the sequences or throw a clear error.
Implement the checks immediately after the New-PortSequence calls (referencing
New-PortSequence, $resolvedSpectatorSignalPorts, $resolvedSpectatorMediaPorts,
$KitSignalPort, $KitMediaPort) and fail fast with a descriptive message (or
recompute ranges) so collisions are prevented before Kit is launched around the
existing startup block that uses those variables.
- Around line 256-257: The refresh gate currently only honors $Build or
$isPublicHostExplicit when setting $shouldRefreshWebPlane; update this logic to
also detect environment-driven topology changes by checking the effective
PUBLIC_HOST/spectator env settings and the resolved conversion bind host (e.g.,
include checks against $env:PUBLIC_HOST, any spectator-related env flags, and
$resolvedConversionBindHost) so that changes loaded from env files trigger a
compose/web-plane refresh; apply the same expanded check where similar logic is
used later (the block around lines 697-699) so both places use the same combined
condition.
In `@scripts/lib/host-native-launcher.ps1`:
- Around line 140-142: The current branch only sets
$env:STREAMING_CONVERSION_PUBLIC_ARTIFACTS_URL when $PublicArtifactsUrl is
non-empty, leaving any previously set value intact; update the logic around
$PublicArtifactsUrl so that when it is null or whitespace you explicitly
clear/unset $env:STREAMING_CONVERSION_PUBLIC_ARTIFACTS_URL (use Remove-Item
Env:STREAMING_CONVERSION_PUBLIC_ARTIFACTS_URL or set it to an empty string) and
otherwise set it to $PublicArtifactsUrl, referencing the $PublicArtifactsUrl
variable and the STREAMING_CONVERSION_PUBLIC_ARTIFACTS_URL environment variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1ff07d0-0e6a-4fd9-a8a4-1573821c25f3
📒 Files selected for processing (22)
.env.web-plane.host-kit.examplebim-review-coordinator/src/config.tsbim-review-coordinator/tests/config.test.tscompose.host-kit.ymldocs/verification/2026-05-27-fix-lan-runtime-params-spectator-capacity.mdopenspec/changes/fix-lan-runtime-params-spectator-capacity/.openspec.yamlopenspec/changes/fix-lan-runtime-params-spectator-capacity/design.mdopenspec/changes/fix-lan-runtime-params-spectator-capacity/proposal.mdopenspec/changes/fix-lan-runtime-params-spectator-capacity/specs/docker-web-plane-host-native-kit/spec.mdopenspec/changes/fix-lan-runtime-params-spectator-capacity/specs/local-coordinator-ifc-ready-intake-boundary/spec.mdopenspec/changes/fix-lan-runtime-params-spectator-capacity/specs/multi-artifact-kit-routing/spec.mdopenspec/changes/fix-lan-runtime-params-spectator-capacity/specs/runtime-verification-task-status/spec.mdopenspec/changes/fix-lan-runtime-params-spectator-capacity/tasks.mdscripts/check-web-plane-docker.ps1scripts/deploy.ps1scripts/lib/host-native-launcher.ps1scripts/lib/preflight-host-native.ps1scripts/lib/preflight-ports.ps1scripts/start-web-plane-docker.ps1scripts/tests/test-host-native-launcher.ps1scripts/tests/test-preflight-host-native.ps1scripts/tests/test-preflight-ports.ps1
Respect explicit public base URLs in deploy output, reject malformed PUBLIC_HOST values, and guard generated spectator ports against primary collisions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23cfde50df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $resolvedSpectatorSignalPorts = @(New-PortSequence -Count $resolvedSpectatorCount -Start $resolvedSpectatorSignalStart -Stride $resolvedSpectatorStride -Name 'KIT_SPECTATOR_SIGNALING_PORT') | ||
| $resolvedSpectatorMediaPorts = @(New-PortSequence -Count $resolvedSpectatorCount -Start $resolvedSpectatorMediaStart -Stride $resolvedSpectatorStride -Name 'KIT_SPECTATOR_MEDIA_PORT') | ||
| $isPublicHostExplicit = -not [string]::IsNullOrWhiteSpace($PublicHost) | ||
| $shouldRefreshWebPlane = $Build -or $isPublicHostExplicit |
There was a problem hiding this comment.
When an operator edits .env.web-plane.host-kit to change PUBLIC_HOST, KIT_SPECTATOR_COUNT, or spectator port settings and reruns deploy while coordinator/viewer containers are already running, this flag remains false unless the value came from the -PublicHost CLI switch or -Build. Phase 4c then skips docker compose up, so the coordinator container keeps its old environment and continues advertising the previous Kit endpoints even though the deploy script resolved the new values. Please include env-file-derived runtime changes in the refresh decision or compare the running container env before skipping.
Useful? React with 👍 / 👎.
| $startInfo = Start-HostNativeConversion ` | ||
| -RepoRoot $RepoRoot ` | ||
| -RuntimeStorageRoot $volume.runtimeStorageRoot ` | ||
| -BindHost $resolvedConversionBindHost ` |
There was a problem hiding this comment.
When -ConversionBindHost is set to a concrete LAN address, this starts the conversion service with STREAMING_CONVERSION_HOST bound to that address, but Phase 4a still verifies http://127.0.0.1:49101/health. Uvicorn bound to a specific non-loopback address does not listen on loopback, so deploy reports a startup timeout even though the service is healthy on the requested bind address; probe the resolved bind host, using loopback only for 0.0.0.0/loopback binds.
Useful? React with 👍 / 👎.
| Set-DeployEnvIfNeeded -Name 'WEB_VIEWER_COORDINATOR_SOCKET_URL' -Value $script:coordinatorPublicUrl -Force:$isPublicHostExplicit -EnvFile $resolvedEnvFile | ||
| Set-DeployEnvIfNeeded -Name 'VIEWER_PUBLIC_BASE_URL' -Value $script:viewerPublicUrl -Force:$isPublicHostExplicit -EnvFile $resolvedEnvFile | ||
| Set-DeployEnvIfNeeded -Name 'COORDINATOR_PUBLIC_BASE_URL' -Value $script:coordinatorPublicUrl -Force:$isPublicHostExplicit -EnvFile $resolvedEnvFile | ||
| Set-DeployEnvIfNeeded -Name 'STREAMING_CONVERSION_PUBLIC_ARTIFACTS_URL' -Value "http://${resolvedPublicHost}:49101/artifacts" -Force:$isPublicHostExplicit -EnvFile $resolvedEnvFile |
There was a problem hiding this comment.
If the operator enables LAN by editing PUBLIC_HOST in .env.web-plane.host-kit, the example file still contains STREAMING_CONVERSION_PUBLIC_ARTIFACTS_URL=http://127.0.0.1:49101/artifacts, so this call treats the loopback value as already configured and does not derive http://<PUBLIC_HOST>:49101/artifacts. The conversion service then publishes loopback artifact URLs in successful conversion results, making remote Kit/viewer handoff fail unless the operator also knows to update this second setting. Please override the default loopback artifact URL when PUBLIC_HOST comes from the env file or make the example leave it unset for derivation.
Useful? React with 👍 / 👎.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
Fail fast on malformed KIT_INSTANCE_ENDPOINTS, guard spectator port collisions during deploy, refresh the web-plane for topology inputs, and clear stale conversion artifact URL env state.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b6e6adc65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| KIT_INSTANCE_ENDPOINTS: >- | ||
| [{"id":"kit_host_native_001","signalingServer":"${KIT_SIGNALING_HOST:-127.0.0.1}","signalingPort":${KIT_SIGNALING_PORT:-49100},"mediaServer":"${KIT_MEDIA_HOST:-127.0.0.1}","mediaPort":${KIT_MEDIA_PORT:-47998}}] | ||
| [{"id":"kit_host_native_001","signalingServer":"${KIT_SIGNALING_HOST:-${PUBLIC_HOST:-127.0.0.1}}","signalingPort":${KIT_SIGNALING_PORT:-49100},"mediaServer":"${KIT_MEDIA_HOST:-${KIT_SIGNALING_HOST:-${PUBLIC_HOST:-127.0.0.1}}}","mediaPort":${KIT_MEDIA_PORT:-47998}}] |
There was a problem hiding this comment.
When an operator uses the documented advanced escape hatch by setting KIT_INSTANCE_ENDPOINTS in .env.web-plane.host-kit, this hard-coded service environment value overwrites it, so the coordinator still receives the generated single primary endpoint and appends default spectators instead of the explicitly configured topology. In the hybrid Docker path inspected here (compose.host-kit.yml), that makes multi-endpoint or no-spectator overrides ineffective; interpolate ${KIT_INSTANCE_ENDPOINTS:-...} here so env-file overrides actually reach loadConfig().
Useful? React with 👍 / 👎.
| VIEWER_PUBLIC_BASE_URL: ${VIEWER_PUBLIC_BASE_URL:-} | ||
| COORDINATOR_PUBLIC_BASE_URL: ${COORDINATOR_PUBLIC_BASE_URL:-} |
There was a problem hiding this comment.
When the hybrid web plane is started directly with scripts/start-web-plane-docker.ps1 or docker compose and COORDINATOR_PORT is changed (for example to 9004), this leaves COORDINATOR_PUBLIC_BASE_URL empty inside the coordinator while PORT remains the container port 8004. loadConfig() then derives redirects and coordinatorApiBase as http://<PUBLIC_HOST>:8004 even though the host publishes 9004:8004; the deploy wrapper masks this by exporting the derived value, but the direct start path does not, so compose should default this env value from ${COORDINATOR_PORT} or pass an equivalent published-port hint.
Useful? React with 👍 / 👎.
| -SignalPort $KitSignalPort ` | ||
| -StreamPort $KitMediaPort ` |
There was a problem hiding this comment.
When .env.web-plane.host-kit changes KIT_SIGNALING_PORT or KIT_MEDIA_PORT, Phase 4b still launches Kit with the PowerShell parameter defaults unless the operator also passes -KitSignalPort / -KitMediaPort. The coordinator container reads those same env-file port values through compose, so deploy can advertise 49200/48200 while the host-native Kit is actually started on 49100/47998; resolve the primary ports from the env file the same way the new spectator ports are resolved before launching and waiting for Kit.
Useful? React with 👍 / 👎.
Track host-native Kit runtime parameters, restart stale Kit processes, pass publicIp to spectator streams, audit media ports as UDP, and derive LAN artifact URLs from PUBLIC_HOST.
Let compose pass KIT_INSTANCE_ENDPOINTS overrides, derive coordinator public URLs from published ports, and resolve primary Kit ports from the selected env file before launch.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f77c00ebd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| -BindHost $resolvedConversionBindHost ` | ||
| -PublicArtifactsUrl ([Environment]::GetEnvironmentVariable('STREAMING_CONVERSION_PUBLIC_ARTIFACTS_URL')) |
There was a problem hiding this comment.
When the conversion service is already running and an operator reruns deploy with a new -PublicHost, -ConversionBindHost, or derived STREAMING_CONVERSION_PUBLIC_ARTIFACTS_URL, Phase 4a takes the existing-PID skip path just above this block, so these newly resolved launch parameters are never applied. That leaves the service bound to the old host and/or still publishing old loopback artifact URLs while the coordinator/Kit topology has moved to the new LAN settings; please track a conversion runtime signature or restart the conversion service when these values change.
Useful? React with 👍 / 👎.
| [Environment]::SetEnvironmentVariable('KIT_SPECTATOR_MEDIA_PORT_START', [string]$resolvedSpectatorMediaStart, 'Process') | ||
| [Environment]::SetEnvironmentVariable('KIT_SPECTATOR_PORT_STRIDE', [string]$resolvedSpectatorStride, 'Process') | ||
| Set-DeployEnvIfNeeded -Name 'VIEWER_BIND_HOST' -Value ($(if (Test-LoopbackHost -HostName $resolvedPublicHost) { '127.0.0.1' } else { '0.0.0.0' })) -Force:$shouldDerivePublicTopologyValues -EnvFile $resolvedEnvFile | ||
| Set-DeployEnvIfNeeded -Name 'KIT_SIGNALING_HOST' -Value $resolvedPublicHost -Force:$shouldDerivePublicTopologyValues -EnvFile $resolvedEnvFile |
There was a problem hiding this comment.
When .env.web-plane.host-kit sets a non-loopback PUBLIC_HOST and also provides a different KIT_SIGNALING_HOST/KIT_MEDIA_HOST for a split-host or reverse-proxy setup, $shouldDerivePublicTopologyValues becomes true and this forced write overwrites the explicit Kit host before docker compose is launched. That contradicts the example guidance to leave these fields unset to derive them from PUBLIC_HOST, and it makes the coordinator advertise PUBLIC_HOST instead of the operator's configured Kit endpoint; only force this on the -PublicHost CLI override path, or derive per variable only when it is unset.
Useful? React with 👍 / 👎.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
變更摘要
本 PR 實作 OpenSpec change
fix-lan-runtime-params-spectator-capacity：把 LAN client handoff 從 loopback 收斂成可設定的 public host/base URL，補上 same-Kit primary + spectator WebRTC endpoint topology，並修復一鍵部署在 Kit runtime build artifacts 缺失時要先執行bim-streaming-server\repo.bat build的問題。主要變更
bim-review-coordinator: 新增/測試 spectator endpoint generation，保留 explicitKIT_INSTANCE_ENDPOINTSoverride。compose.host-kit.yml/.env.web-plane.host-kit.example: 傳入PUBLIC_HOST、public coordinator/viewer base、viewer bind host、Kit host 與 spectator 參數。scripts/deploy.ps1: 支援-PublicHost、spectator port topology、LAN summary URL；Phase 2 會在 Kit_buildartifacts 缺失時自動跑repo.bat build，失敗則早停並指向scripts\.run\kit-repo-build.log。scripts/lib/preflight-host-native.ps1: 新增kitRuntime=OK|NEEDS_BUILD、kitBuildRequired、runtime launcher/kit.exe path 與 build command evidence。docs/verification/2026-05-27-fix-lan-runtime-params-spectator-capacity.md：記錄需求、設計、驗證與 GitNexus worktree 限制。驗證方式
openspec validate fix-lan-runtime-params-spectator-capacity --strictpowershell.exe -NoProfile -ExecutionPolicy Bypass -File scripts\tests\test-preflight-host-native.ps1powershell.exe -NoProfile -ExecutionPolicy Bypass -File scripts\tests\test-deploy-dryrun.ps1powershell.exe -NoProfile -ExecutionPolicy Bypass -File scripts\tests\test-preflight-ports.ps1powershell.exe -NoProfile -ExecutionPolicy Bypass -File scripts\tests\test-host-native-launcher.ps1npm test -- --run tests/config.test.ts tests/sessions.test.tsinbim-review-coordinator— 52 passednpm run buildinbim-review-coordinatornpm run test:session-firstinweb-viewer-samplenpm run buildinweb-viewer-sample.\.venv\Scripts\python.exe -m pytest tests -x -q— 65 passed..\.venv\Scripts\python.exe -m pytest tests/test_conversion_authority_api.py -x -q— 14 passeddocker compose -f compose.runtime-manager.yml -f compose.host-kit.yml --env-file .env.web-plane.host-kit.example config --quietgit diff --cached --check風險與影響
impactonscripts/deploy.ps1andscripts/lib/preflight-host-native.ps1returned LOW/no indexed callers.detect_changescurrently cannot inspect this unindexed.worktreescheckout and reports no changes against indexed main; fallback evidence is documented indocs/verification/2026-05-27-fix-lan-runtime-params-spectator-capacity.md.web-viewer-sampledependency install reported existing npm audit findings and Node 22 engine warning; dependencies were not changed in this PR.回滾方式
若 merge 後出問題：
gh pr revert <pr-number>開 revert PR，或 revert commit556cc70。Runtime 驗收
Merge 後仍需在主工作區跑
.\scripts\deploy.ps1 -Build -PublicHost 192.168.10.105，重新 POST IFC-ready，並用同一 session 驗證 primary viewer + spectator viewer 都達到 live WebRTC video readiness。Summary by CodeRabbit
New Features
Improvements
Documentation