Skip to content

fix: six defects found in the pre-1.0 review (1.0.0) - #236

Merged
facontidavide merged 2 commits into
mainfrom
fix/pre-1.0-bugfixes
Nov 14, 2026
Merged

facontidavide merged 2 commits into
mainfrom
fix/pre-1.0-bugfixes

Conversation

@facontidavide

Copy link
Copy Markdown
Contributor

What

Six defects from the pre-1.0 review. None changes an ABI layout, a wire format or a public signature in a source-incompatible way. Each code fix has one regression test that was seen failing before the fix.

# Defect Fix
1 toStringView returned a view of size bytes past a one-byte literal for {nullptr, size > 0}. It is applied to plugin-supplied views in the host-side adapters. Returns an empty view when the pointer is null. The existing test accepted the bug and is now stricter.
2 The dialog marker decoders (chartMarkers(), timelineMarksFromJson) used nlohmann's throwing value(). A plugin sending {"kind": 1} threw through the host, which calls them without a try/catch. They skip the malformed entry, as the sibling list decoders (chartSeries, rangeSliderMarkers) do.
3 DialogHandle kept a null context when the plugin's create() failed, then passed it to plugin trampolines that dereference it. New valid(); each method returns its empty value without calling the plugin. Before the fix the new test hit a null dereference under ASAN.
4 One non-UTF-8 string made WidgetData::toJson() throw; the trampoline then replaced the whole update with {}, silently. Invalid bytes become U+FFFD. The 31 WidgetEventBuilder serializers had the same unguarded dump() and use the same helper.
5 The Conan assert_throws option did not reach consumers, so their inline PJ_ASSERT disagreed with the archives (ODR). package_info() adds the define to the base component, as the conan-center recipe already did.
6 Builtin object types were invisible to the feature-floor checker, so a plugin emitting MeshMap with a floor below 1.0.0 passed. The extractor now emits PJ_builtin_object_type_t enumerators; feature_floors.json gains a 1.0.0 group for kMeshMap and kMeshMapUpdate.

Things a reviewer should know

  • Fix 2 removes a duplicate. chartMarkersFromJson had no callers while the host view re-implemented it, despite a comment calling it the single source of truth. The view now calls it, so ChartMarkerView is an alias of ChartMarker (same fields, order and defaults). PlotJuggler only uses the result through auto.
  • Fix 2 is slightly stricter. A timeline mark with a float in an integer field (start: 3.7) used to be truncated and is now skipped. The SDK's writers always emit integers.
  • Fix 6, baseline. All 18 pre-existing types are classified as baseline. I checked against git tags that each first shipped by 0.26.0, below the 0.28.0 minimum floor, so that is accurate.
  • Fix 6, over-matching. The aliases MeshMap / kMeshMap also match a plugin that only reads meshes, which would work on an older host. It will be asked to raise its floor. That was chosen over letting an emitting plugin slip through.
  • Fix 6, changelog. The two sentences in the 1.0.0 Host contract: paragraph that said the checker cannot see builtin types are rewritten.

PlotJuggler follow-up (optional)

pj_dialog_host/src/dialog_engine.cpp constructs a parser DialogHandle and uses it at once. With fix 3 that no longer crashes, but a dialog whose constructor threw would show an empty UI. It should check valid().

Verification

  • ./build.sh --debug and ./test.sh (Debug + ASAN): 107 of 107 pass
  • check_feature_floors.py: OK, 162 surfaces. test_declared_versions.py: 7 tests pass, including a new MeshMapFloorTest
  • pre-commit clean
  • Fix 5: conan create with assert_throws=True, then a consumer that fails to compile without PJ_ASSERT_THROWS: passes

Rebased onto main after the 1.0.0-dev.1 commit; not rebuilt after the rebase. This PR and #230/#231 edit nearby lines in feature_floors.json, extract_host_surfaces.py and conanfile.py; whichever merges later needs a small rebase.

🤖 Generated with Claude Code

@facontidavide
facontidavide force-pushed the fix/pre-1.0-bugfixes branch 2 times, most recently from 5bd1e86 to f8ee1c8 Compare November 13, 2026 23:22
facontidavide and others added 2 commits November 14, 2026 00:52
- toStringView returned a view of `size` bytes past a one-byte literal for a
  null pointer with a non-zero size; it now returns an empty view.
- The dialog marker decoders (chart markers, timeline marks) threw on a
  wrong-typed field, which the host calls without a try/catch. They now skip
  the malformed entry, like their sibling list decoders. The host view reuses
  the shared decoder instead of a private copy; ChartMarkerView is an alias of
  ChartMarker.
- DialogHandle kept a null context when the plugin's create() failed and then
  passed it to the plugin. It gains valid() and its methods return their empty
  value without calling the plugin.
- One non-UTF-8 string made WidgetData::toJson() throw, and the trampoline
  replaced the whole update with {}. Invalid bytes now become U+FFFD.
- The Conan assert_throws option did not reach consumers, so their inline
  PJ_ASSERT disagreed with the archives.
- Builtin object types are now host surfaces in the feature-floor table, so a
  plugin emitting MeshMap or MeshMapUpdate must declare min_sdk_required 1.0.0.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…w floor surface

toStringView was fixed, but four host-side readers of plugin-supplied views
carried their own copy of the same pattern and still read `size` bytes past a
literal: the service-registry lookup and the saveConfig() readers of the
DataSource, MessageParser and Toolbox handles (plus the parser write recorder
used in tests). A registry lookup with {nullptr, 64} was an ASAN
global-buffer-overflow; it is now an unknown service.

The changelog says so, and notes that an integer marker field no longer
accepts a float. ARCHITECTURE and the plugin skill list builtin object types
among the surfaces that set a host floor.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@facontidavide
facontidavide merged commit f8bf2b6 into main Nov 14, 2026
4 checks passed
@facontidavide
facontidavide deleted the fix/pre-1.0-bugfixes branch November 14, 2026 00:05
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