Support explicit debugger ports - #155
Conversation
📊 PR Size: size/LTotal changes: 796 lines (22 files) Top files changed:
...and 12 more files Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+) |
🎉 Bazel & CI Test Results
All Bazel configuration and CI tests passed! ✨ The build system and core tooling are working correctly. 🚀 Bazel remote cache is now enabled - future builds will be faster! Workflow: Valdi CI |
clholgat
left a comment
There was a problem hiding this comment.
Reviewed the explicit-port work against #154. The validation itself looks good — range checks, value-redacted logging, legacy constructor/resolver preserved, and solid native test coverage. One cross-PR contract issue on the env-var naming; see inline.
Note: as this 24-PR stack will be squashed into ~3 PRs, please carry this feedback into the squashed PRs.
| resolution.rejectedRequestedPort = requestedPort; | ||
| } | ||
|
|
||
| const char* overridePort = std::getenv("VALDI_DEBUGGER_PORT"); |
There was a problem hiding this comment.
🔴 High (cross-PR): This reads VALDI_DEBUGGER_PORT to set the on-device debugger TCP service port (default 13591/13592), but the CLI debugger (PR #154, server.ts) reads the same env var to set its browser UI HTTP port (default 8765) — the variable is overloaded to mean two different ports. Worse, #154's target discovery hardcodes 13591/13592 and never reads this var, so relocating the service port here is not picked up by the debugger UI (only a manual ?port= works). Failure scenario: export VALDI_DEBUGGER_PORT=14000 to move the service → the CLI UI also mis-binds to 14000 and still probes 13591/13592. Suggest distinct names (VALDI_DEBUGGER_SERVICE_PORT vs VALDI_DEBUGGER_UI_PORT) and having #154 discovery honor the configured service port.
Description
Adds validated explicit debugger-port selection through runtime and platform bootstrap APIs while preserving the existing entry points and platform defaults.
Type of Change
Testing
bazel test //...)Testing Details
npm testpassed 436/436; the CLI production build passed.//src/valdi_modules/src/valdi/web_renderer:testpassed.bazel query //...passed.Checklist
Related Issues
Relates to #154
Additional Context
Stack 2/22. Stacked on #154 (
bjd/upstream-debugger-foundation). Review this PR as the single incremental commit10af426aagainst that base; do not merge it before its parent.