Skip to content

Add the exact component-property edit protocol - #173

Closed
bjdodson-openai wants to merge 1 commit into
bjd/debugger-component-propertiesfrom
bjd/debugger-component-property-edit-protocol
Closed

Add the exact component-property edit protocol#173
bjdodson-openai wants to merge 1 commit into
bjd/debugger-component-propertiesfrom
bjd/debugger-component-property-edit-protocol

Conversation

@bjdodson-openai

Copy link
Copy Markdown
Collaborator

Description

Adds web-only, revision-bound scalar mutation authorization and a descriptor-stable renderer update path.

  • Issues 128-bit, single-use tokens bound to exact target, component, property, type, and value revisions.
  • Rejects stale identity, accessors, functions, inherited or forbidden properties, and non-finite values.
  • Uses a shadow overlay that preserves source descriptors, prototypes, receiver semantics, and frozen or sealed invariants.

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: Protocol boundary passed CLI 402/402 and production build; frozen-patch independent reviews, TypeScript, lint, format, syntax, 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 20/22. Stacked on #172 (bjd/debugger-component-properties). Review this PR as the single incremental commit 2dd01e3c against that base; do not merge it before its parent.

@github-actions

Copy link
Copy Markdown

📊 PR Size: size/XL

Total changes: 3261 lines (12 files)

Top files changed:

  • src/valdi_modules/src/valdi/valdi_test/test/Renderer.spec.ts: +929 -0
  • src/valdi_modules/src/valdi/web_renderer/test/WebDebuggerBridge.spec.ts: +699 -7
  • src/valdi_modules/src/valdi/web_renderer/src/debug/WebDebuggerBridge.ts: +375 -4
  • src/valdi_modules/src/valdi/valdi_core/src/Renderer.ts: +362 -1
  • npm_modules/cli/src/debugger/server.spec.ts: +278 -4
  • src/valdi_modules/src/valdi/web_renderer/test/LegacyWebDebuggerAdapter.spec.ts: +214 -0
  • src/valdi_modules/src/valdi/web_renderer/src/debug/ComponentHierarchySnapshot.ts: +209 -4
  • npm_modules/cli/src/debugger/server.ts: +129 -3
  • src/valdi_modules/src/valdi/web_renderer/src/ValdiWebRendererDelegate.ts: +18 -3
  • src/valdi_modules/src/valdi/valdi_core/src/IRenderer.ts: +14 -0

...and 2 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
Snapshot Tests ✅ success
Linux: Registry Validation ✅ success
Test Coverage Delta ✅ success
Valdi Smoke Tests ✅ success
valdi_web Integration Test ✅ success
macOS: C++ & Platform Tests ❌ failure
Linux: Build & Export ✅ success
Linux: Module Tests ✅ success
API Surface Check ✅ success
Linux: Hotreload Smoke ✅ success
Linux: Build Compiler ✅ success
Linux: C++ Tests ✅ 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.

Review of the component-property edit protocol. The write path is well-bounded overall (single-use 128-bit tokens bound to target/component/property/type/revision, three-layer type+range validation, descriptor-stability double-capture, scalar-only, native targets structurally read-only). One design concern noted inline about the wire contract's forward/backward compatibility.

Note for the squash: please carry this feedback into the squashed PR that contains the edit protocol.

'value',
];
const keys = Object.keys(body).sort();
if (keys.length !== expectedKeys.length || keys.some((key, index) => key !== expectedKeys[index])) {

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 — the edit wire contract is validated by exact key-set equality (and the bridge's parseComponentPropertyEditRequest does the same) with no protocol version field. But the bridge lives inside the inspected app (which can be any Valdi version), while the server/client are the independently-versioned CLI. Any future addition to the request tuple becomes a lock-step-only change with no negotiation: a newer CLI editing an app built with an older bridge (or vice-versa) will fail closed on the strict key-set check with no way to degrade gracefully. Failure scenario: a later release adds an optional field (e.g. an idempotency key) → every cross-version edit silently 400s/409s even though both sides support editing. Suggested fix: add an explicit protocolVersion field to the tuple now (accepted by both server and bridge), and validate a known-version key set per version rather than one frozen exact set, so the contract can evolve without a coordinated bridge+CLI upgrade.

@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