Skip to content

Add unit tests and code review for Moltbot.Shared#1

Merged
shanselman merged 4 commits intomasterfrom
copilot/review-and-add-unit-tests
Jan 29, 2026
Merged

Add unit tests and code review for Moltbot.Shared#1
shanselman merged 4 commits intomasterfrom
copilot/review-and-add-unit-tests

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 29, 2026 •
edited
Loading

Code Review Findings

Analyzed Moltbot.Shared library for correctness and identified 7 issues:

Medium Priority

  • JSON parsing in ParseSessions() handles 2 format variations with ad-hoc validation; fragile to schema changes
  • Reconnection triggered from multiple paths (CheckHealthAsync, ListenForMessagesAsync) without coordination; potential rapid retry loops
  • Auth token stored plaintext in settings.json; recommend DPAPI encryption

Low Priority

  • Error handling inconsistent: RequestUsageAsync() silently swallows exceptions, others log/rethrow
  • Missing input validation: no message length limits before WebSocket send
  • Complex session detection logic: IsMain derived from multiple key patterns (":main", ":main:main")
  • Notification classification hardcoded keywords; false positives likely

Full analysis in CODE_REVIEW.md.

Test Coverage

Added 88 unit tests (xUnit, .NET 9.0):

AgentActivityTests       13 tests  → Glyph mapping, DisplayText formatting
ChannelHealthTests       23 tests  → Status display (ON/OFF/ERR/LINKED), capitalization
SessionInfoTests         22 tests  → DisplayText, ShortKey extraction variations
GatewayUsageInfoTests    10 tests  → Token formatting (K/M suffixes), cost display
GatewayClientTests       20 tests  → Notification/tool classification, path utilities

Coverage highlights:

  • All data model display logic (ActivityKind glyphs, status text, token formatting)
  • Notification classification (health/urgent/email/calendar/build types)
  • Tool-to-activity mapping (exec/read/write/edit/search/browser)
  • Edge cases (empty strings, unicode paths, long keys, OS-specific Path.GetFileName())

Not covered (requires integration tests):

  • WebSocket protocol flow
  • Reconnection backoff timing
  • Concurrent event handling

Security

CodeQL scan: 0 vulnerabilities

Documentation

  • CODE_REVIEW.md - Issue analysis with severity ratings and recommendations
  • TEST_COVERAGE.md - Test statistics and coverage breakdown
  • tests/Moltbot.Shared.Tests/README.md - Test usage guide

All tests passing. Codebase is production-ready with documented improvement paths.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits January 29, 2026 02:00
@shanselman
Add comprehensive unit tests for Moltbot.Shared library
d8e6938 
Co-authored-by: shanselman <2892+shanselman@users.noreply.github.com>
@shanselman
Fix .NET version inconsistencies and improve test documentation
01e81bc 
Co-authored-by: shanselman <2892+shanselman@users.noreply.github.com>
@shanselman
Add test coverage summary and complete code review
ce82916 
Co-authored-by: shanselman <2892+shanselman@users.noreply.github.com>
Copilot AI changed the title [WIP] Review code for correctness and add unit tests Add unit tests and code review for Moltbot.Shared Jan 29, 2026
Copilot AI requested a review from shanselman January 29, 2026 02:10
@shanselman shanselman merged commit ce82916 into master Jan 29, 2026
codemonkeychris referenced this pull request in codemonkeychris/openclaw-windows-node Apr 27, 2026
@codemonkeychris
test: cover canvas.a2ui.dump and canvas.caps in CanvasCapability tests
fa378bc 
The new canvas.a2ui.dump and canvas.caps commands shipped without unit
coverage in the capability layer. Adds CanHandle assertions plus
execution-path tests for both: not-open error paths, success paths
returning structured JSON payloads (not quoted strings), and the
default-when-no-handler shape that canvas.caps returns.

Closes the test gap flagged in local-mcp-a2ui-closeout.md item #1.
codemonkeychris referenced this pull request in codemonkeychris/openclaw-windows-node Apr 27, 2026
@codemonkeychris
test: cover canvas.a2ui.dump and canvas.caps in CanvasCapability tests
e12debb 
The new canvas.a2ui.dump and canvas.caps commands shipped without unit
coverage in the capability layer. Adds CanHandle assertions plus
execution-path tests for both: not-open error paths, success paths
returning structured JSON payloads (not quoted strings), and the
default-when-no-handler shape that canvas.caps returns.

Closes the test gap flagged in local-mcp-a2ui-closeout.md item #1.
codemonkeychris referenced this pull request in codemonkeychris/openclaw-windows-node Apr 27, 2026
@codemonkeychris
test: cover canvas.a2ui.dump and canvas.caps in CanvasCapability tests
002d5ee 
The new canvas.a2ui.dump and canvas.caps commands shipped without unit
coverage in the capability layer. Adds CanHandle assertions plus
execution-path tests for both: not-open error paths, success paths
returning structured JSON payloads (not quoted strings), and the
default-when-no-handler shape that canvas.caps returns.

Closes the test gap flagged in local-mcp-a2ui-closeout.md item #1.
codemonkeychris referenced this pull request in codemonkeychris/openclaw-windows-node Apr 27, 2026
@codemonkeychris
test: cover canvas.a2ui.dump and canvas.caps in CanvasCapability tests
df29a1a 
The new canvas.a2ui.dump and canvas.caps commands shipped without unit
coverage in the capability layer. Adds CanHandle assertions plus
execution-path tests for both: not-open error paths, success paths
returning structured JSON payloads (not quoted strings), and the
default-when-no-handler shape that canvas.caps returns.

Closes the test gap flagged in local-mcp-a2ui-closeout.md item #1.
codemonkeychris referenced this pull request in codemonkeychris/openclaw-windows-node Apr 28, 2026
@codemonkeychris
test: cover canvas.a2ui.dump and canvas.caps in CanvasCapability tests
16f8d15 
The new canvas.a2ui.dump and canvas.caps commands shipped without unit
coverage in the capability layer. Adds CanHandle assertions plus
execution-path tests for both: not-open error paths, success paths
returning structured JSON payloads (not quoted strings), and the
default-when-no-handler shape that canvas.caps returns.

Closes the test gap flagged in local-mcp-a2ui-closeout.md item #1.
@shanselman shanselman mentioned this pull request Apr 29, 2026
Merged
7 tasks
AlexAlves87 pushed a commit to AlexAlves87/openclaw-windows-node that referenced this pull request May 4, 2026
@claude
security: tighten -EncodedCommand detection in ResolveForAllowlist
99cc2a9 
Three gaps addressed (Hanselman review finding openclaw#1):

- SegmentUsesEncodedCommand now extracts quoted tokens as a unit so
  `"-enc"` and `'-enc'` are detected alongside the unquoted forms.
- IsEncodedCommandFlag handles colon/equals separators
  (-EncodedCommand:payload) and any unambiguous prefix abbreviation of
  -encodedcommand longer than -enc (e.g. -encod, -encode).
- DirectExecUsesEncodedCommand added: direct top-level invocations
  ["powershell", "-enc", "..."] and transparent-env-wrapped forms
  ["env", "pwsh", "-enc", "..."] were not checked at all before; both
  now return [] from ResolveForAllowlist.

Tests added: direct -EncodedCommand, -ec alias, -enco abbreviation,
powershell.exe suffix, transparent env+pwsh, quoted flag in segment,
colon-separator form, and a non-fail-closed guard for -Command.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AlexAlves87 AlexAlves87 mentioned this pull request May 4, 2026
Open
@github-actions github-actions Bot mentioned this pull request May 6, 2026
Open
RBrid added a commit to RBrid/openclaw-windows-node that referenced this pull request May 6, 2026
@RBrid
audio: SHA-256 verification of Whisper models and Piper voices
f8facfb 
Critical (rubber-duck openclaw#1) — fail-closed integrity check before install.

* New `Sha256` field on `WhisperModelInfo` and `PiperVoiceInfo`.
* All 9 catalog entries (3 Whisper models + 6 Piper voices) carry a
  pinned lowercase-hex SHA-256, captured against the live HuggingFace
  and sherpa-onnx GitHub releases on 2026-05-05.
* Download core methods now:
    1. Refuse outright if the catalog entry has no pinned hash
       (`InvalidOperationException`).
    2. Compute SHA-256 of the temp file BEFORE the atomic rename
       (Whisper) or BEFORE the tar extraction (Piper).
    3. On mismatch, throw `System.Security.SecurityException`,
       delete the temp file, and let the catch block tear down any
       half-installed directory. Sanitized message — does NOT echo
       the actual hash (no confirmation oracle).
* New `AssetHashPinningTests` enforces that every catalog entry has
  a 64-hex-char SHA-256 and an https URL — future additions that
  forget the hash now break the build.

Audio_FollowUps.md §2 updated:
* Status block at the top documents what landed today.
* Pre-public-release TODO list trimmed to: independent re-verification
  of the pinned hashes, on-load verification (not just on download),
  and a future signed-manifest format so updates don't require a tray
  rebuild. The original detailed design notes are preserved as the
  spec for that next iteration.

Tests: Shared 1275 / Tray 462. Build green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shanselman shanselman Awaiting requested review from shanselman

Assignees

@shanselman shanselman

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@shanselman