Skip to content

Visualizer testing - #456

Open
noahlevenson wants to merge 1 commit into
mainfrom
nelson/visualizer-testing
Open

noahlevenson wants to merge 1 commit into
mainfrom
nelson/visualizer-testing

Conversation

@noahlevenson

@noahlevenson noahlevenson commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Prob want to merge this eventually - it fixes a logging regression for wasm targets that was introduced back in May.

Summary by CodeRabbit

  • Improvements
    • Default diagnostic messages in the WebAssembly client now appear on stderr and include debug-level output.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d8e563fa-b64d-42c8-afba-65e484cfd356
📥 Commits

Reviewing files that changed from the base of the PR and between 06579d8 and 90ad9c6.

⛔ Files ignored due to path filters (1)
  • ui/public/widget.wasm is excluded by !**/*.wasm
📒 Files selected for processing (1)
  • cmd/client_wasm_impl.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The WASM entry point now configures the default slog logger to write text-formatted output to stderr at debug level.

Changes

WASM logging

Layer / File(s) Summary
Configure the WASM logger
cmd/client_wasm_impl.go
The WASM entry point configures the default logger to write text-formatted output to stderr at debug level.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 90ad9

The WASM debug message reaches the browser console, and this change does not alter logging for non-WASM entrypoints. No actionable merge risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 90ad9

Debug logging is enabled for every WASM instance and can expose connection diagnostics and rejected signaling messages in the browser console. No new permissions or remote access capability were identified. Visibility beyond the browser context and the contents of the updated prebuilt binary remain unverified.

Retained concerns

  • Low · security · inferred: Unconditional process-wide debug logging expands browser-console visibility of connection diagnostics and complete rejected signaling payloads. This creates a confidentiality exposure if console output is captured or shared beyond the session’s browser context; such downstream access has not been established.
Security review details

Security Blast Radius

  • inferred — The supported exposure is diagnostic output from each WASM runtime using this source policy, including its connection attempts. Documented shared publication can distribute that policy to embed consumers. No cross-tenant data-store access or privilege escalation is established.

Security Findings and Attack Paths

  • inferred — Malformed peer or discovery signaling responses can enter debug output when decoding fails. The new level makes those unchanged payload-bearing call sites emit by default. Access by an unauthorized reader would additionally require access to the browser context or captured logs; no remote extraction path or actual credential disclosure was verified.

Trust Boundaries and Controls

  • observed — Rejected signaling content is copied into log records after decoding failure. The new handler has no application redaction hook configured. The browser console sink is conditional: an already supplied global filesystem bypasses the checked-in fallback, and its output behavior was not established.

Resilience and Maintainability Implications

  • inferred — The logging-policy transition is synchronous and precedes API registration. Failed construction retains the same valid logger, and repeated constructor calls do not reconfigure it. No additional logger-specific partial or recovery state was identified beyond the process-lifetime debug policy.

Hardening Proposals

  • proposed — Consider an explicit diagnostic opt-in and payload minimization for signaling bodies and candidate details, particularly before integrating browser-console collection. These are mitigation proposals, not evidence of an existing remote disclosure.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title "Visualizer testing" does not describe the main change, which fixes default slog output for WebAssembly targets. Use a title that identifies the logging fix, such as "Fix default WASM logging output".
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant