Skip to content

Add integrated debugger DevTools core - #158

Closed
bjdodson-openai wants to merge 1 commit into
bjd/debugger-web-bridgefrom
bjd/debugger-devtools-core
Closed

Add integrated debugger DevTools core#158
bjdodson-openai wants to merge 1 commit into
bjd/debugger-web-bridgefrom
bjd/debugger-devtools-core

Conversation

@bjdodson-openai

Copy link
Copy Markdown
Collaborator

Description

Adds the top-level browser DevTools surface, bounded tree model, Owl and Chromium clients, loopback routes, and package integration.

  • Projects untrusted runtime values through bounded, non-recursive presentation helpers.
  • Enforces loopback and origin policy for browser routes.
  • Packages the panel assets and client modules in the CLI distribution.

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: Focused DevTools coverage passed 101/101; integrated CLI 230/230, strict TypeScript, package dry-run, and production build 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 5/22. Stacked on #157 (bjd/debugger-web-bridge). Review this PR as the single incremental commit 5f768c6f against that base; do not merge it before its parent.

@github-actions

Copy link
Copy Markdown

📊 PR Size: size/XL

Total changes: 6888 lines (26 files)

Top files changed:

  • npm_modules/cli/src/debugger/debuggerTreeModel.spec.ts: +896 -0
  • npm_modules/cli/src/debugger/server.ts: +816 -47
  • npm_modules/cli/debugger/devtools-panel.js: +742 -0
  • npm_modules/cli/src/utils/owlCdpClient.spec.ts: +662 -0
  • npm_modules/cli/debugger/devtools-panel.css: +610 -0
  • npm_modules/cli/debugger/debugger-tree-model.js: +539 -0
  • npm_modules/cli/src/debugger/server.spec.ts: +434 -1
  • npm_modules/cli/src/utils/chromiumDevToolsClient.ts: +324 -0
  • npm_modules/cli/src/utils/owlCdpClient.ts: +310 -0
  • npm_modules/cli/src/utils/hermesClient.ts: +81 -205

...and 16 more files

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

@github-actions github-actions Bot added the area/docs Documentation label Aug 26, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Bazel & CI Test Results

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

Some tests failed. Please check the workflow logs for details.

🚀 Bazel remote cache is now enabled - future builds will be faster!

Workflow: Valdi CI

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

DevTools core review. One medium item inline; one low item below.

🟢 Low — serveStatic (server.ts ~2243-2246) stops sending X-Frame-Options: DENY for devtools-panel.html and relaxes CSP frame-ancestors to chrome-extension://*. That permits any installed Chromium extension (not only the ephemeral Valdi one) to embed the loopback panel. Framing risk is low here (an embedder still lacks the target nonce and cross-origin reads are blocked), but it would be cleaner to pin to the specific extension origin if the unpacked-extension ID allows it, or add a comment explaining why the wildcard is required.

Note: this stack is being squashed to ~3 PRs — please carry this feedback into whichever squashed PR these changes land in.

if (expression.length > 10_000) {
throw new ApiRequestError(400, 'Web preview console expressions cannot exceed 10,000 characters.');
}
const wrapped =

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.

🟠 Med — this endpoint forwards a caller-supplied expression into the inspected application for evaluation. Its only real authorization is the page-injected __VALDI_DEVTOOLS_TARGET_NONCE__ guard; the loopback binding and the application/json content-type requirement are useful defense-in-depth but are not authorization.

Concern: if that nonce is ever logged, shortened, made predictable, or the page-side guard is dropped during a refactor, this path silently loses its only protection.

Suggested fix: add an explicit comment here (and at the guard in owlCdpClient) stating that the per-session nonce is the sole authorization boundary and that its entropy and secrecy must be preserved, so a future change does not weaken it unintentionally.

@bjdodson-openai

Copy link
Copy Markdown
Collaborator Author

Superseded by #180, which consolidates this patch into the reviewed debugger capabilities landing unit. The replacement carries forward the feedback and fixes discussed here. Closing this draft to reduce the active stack; this PR and its discussion remain the historical review record.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants