Skip to content

Support explicit debugger ports - #155

Draft
bjdodson-openai wants to merge 1 commit into
bjd/upstream-debugger-foundationfrom
bjd/debugger-explicit-port
Draft

Support explicit debugger ports#155
bjdodson-openai wants to merge 1 commit into
bjd/upstream-debugger-foundationfrom
bjd/debugger-explicit-port

Conversation

@bjdodson-openai

Copy link
Copy Markdown
Collaborator

Description

Adds validated explicit debugger-port selection through runtime and platform bootstrap APIs while preserving the existing entry points and platform defaults.

  • Preserves legacy constructor and resolver contracts while adding explicit configuration.
  • Handles Android and iOS bootstrap values without logging supplied values.
  • Adds focused native registration, validation, and logging coverage.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation improvement
  • Performance optimization
  • Test improvement
  • Other (new debugger capability)

Testing

  • Tests pass locally (bazel test //...)
  • Added/updated tests for changes (if applicable)
  • Tested on multiple platforms (iOS/Android/Web/macOS as applicable)
  • Manual testing performed (describe below)

Testing Details

  • Incremental branch: Native registration and logging harnesses passed; focused TypeScript, formatting, and diff checks passed.
  • Assembled debugger stack: npm test passed 436/436; the CLI production build passed.
  • Focused //src/valdi_modules/src/valdi/web_renderer:test passed.
  • bazel query //... passed.
  • The broad Valdi suite reproduced the established 12 unrelated failures; all new debugger specs passed.

Checklist

  • Code follows project style guidelines
  • Documentation updated (if needed)
  • No breaking changes (or documented in description)
  • Commit messages follow conventional format
  • No secrets, API keys, or internal URLs included

Related Issues

Relates to #154

Additional Context

Stack 2/22. Stacked on #154 (bjd/upstream-debugger-foundation). Review this PR as the single incremental commit 10af426a against that base; do not merge it before its parent.

@github-actions github-actions Bot added area/runtime Valdi runtime (C++/native) area/build-system Bazel build rules and config platform/ios iOS-specific platform/android Android-specific size/L labels Aug 26, 2026
@github-actions

Copy link
Copy Markdown

📊 PR Size: size/L

Total changes: 796 lines (22 files)

Top files changed:

  • valdi/test/java/support/AppBootstrapActivityTest.kt: +161 -0
  • valdi/test/runtime/DebuggerService_tests.cpp: +143 -0
  • valdi/test/ios/SCValdiRuntimeTests.mm: +133 -0
  • valdi/test/runtime/RuntimeManager_tests.cpp: +85 -0
  • valdi/src/valdi/runtime/RuntimeManager.cpp: +57 -3
  • valdi/src/android_support/java/com/snap/valdi/support/AppBootstrapActivity.kt: +48 -0
  • valdi/src/valdi/runtime/Debugger/DebuggerService.cpp: +48 -0
  • valdi_core/src/valdi_core/ios/valdi_core/SCValdiConfiguration.h: +25 -0
  • valdi/src/valdi/ios/SCValdiRuntimeManager.mm: +19 -3
  • valdi/src/valdi/runtime/Debugger/DebuggerService.hpp: +15 -0

...and 12 more files

Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+)

@github-actions

Copy link
Copy Markdown

🎉 Bazel & CI Test Results

Test Suite Result
macOS: C++ & Platform Tests ✅ success
API Surface Check ✅ success
Valdi Smoke Tests ✅ success
Linux: Build & Export ✅ success
Test Coverage Delta ✅ success
Linux: Module Tests ✅ success
Linux: C++ Tests ✅ success
Snapshot Tests ✅ success
valdi_web Integration Test ✅ success
Linux: Build Compiler ✅ success
Linux: Registry Validation ✅ success
Linux: Hotreload Smoke ✅ success

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 clholgat mentioned this pull request Aug 27, 2026
14 tasks

@clholgat clholgat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/build-system Bazel build rules and config area/runtime Valdi runtime (C++/native) platform/android Android-specific platform/ios iOS-specific size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants