Skip to content

Improve MCP validation efficiency and annotate public APIs with nodiscard - #3024

Open
HarshPopat23 wants to merge 1 commit into
sourcemeta:mainfrom
HarshPopat23:perf/mcp-reduce-temporary-allocations
Open

HarshPopat23 wants to merge 1 commit into
sourcemeta:mainfrom
HarshPopat23:perf/mcp-reduce-temporary-allocations

Conversation

@HarshPopat23

@HarshPopat23 HarshPopat23 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Combines MCP validation performance and API safety improvements:

  • Validation Efficiency & Reduced Allocations:
    • Replaced the ad-hoc strings() array-checking loop in src/core/mcp/validation.h with native member function is_array_of_strings() across �udience, sizes, stopSequences, and task environment object values.
    • Eliminated redundant string copies on dispatch (const auto &name{...to_string()}) in sampling_block(), sampling_message_block(), �alid_content(), and �alid_input_requests().
    • Reused bound string references across �nnotations() and icons() loops.
    • Retained exact JSON number comparisons for annotation priority bounds (*priority < JSON{0} || *priority > JSON{1}) to preserve Decimal precision without rounding or overflow.
  • Annotate Public MCP Functions with [[nodiscard]]:
    • Added [[nodiscard]] to all public query, validation, parsing, and factory functions across mcp_protocol.h, mcp_headers.h, mcp_results.h, mcp_capabilities.h, mcp_error.h, and mcp_logging.h to prevent callers from silently ignoring validation results or responses (excluding void-returning mcp_write_result).
    • Updated est/mcp/mcp_helpers.h (EXPECT_MCP_REJECTS) and est/mcp/mcp_error_test.cc to cast discarded expressions to �oid for -Werror=unused-result compliance.

Testing

  • Full MSYS2 UCRT64 GCC 14.2 & Ninja build cleanly passing.
  • 100% pass on all test suites:
    • core.mcp: all 1,409 tests passed, 0 failed.
    • core.json: passed.
    • core.jsonrpc: passed.
  • Code formatted with repository's clang-format configuration.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread src/core/mcp/validation.h Outdated
@augmentcode

augmentcode Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: Improves MCP validation efficiency and makes ignored public API results visible.

  • Replaces the local string-array validator with JSON::is_array_of_strings().
  • Applies it to audiences, icon sizes, sampling stop sequences, and elicitation content arrays.
  • Borrows strings during sampling, content, and nested-request dispatch instead of copying them.
  • Reuses string references when checking audience roles and icon themes.
  • Checks nonnegative resource sizes using is_integral() and is_positive().
  • Preserves exact JSON-number comparisons for annotation priority bounds.
  • Adds [[nodiscard]] across protocol, header, result, capability, error, and logging APIs.
  • Leaves the void-returning mcp_write_result() unannotated.
  • Explicitly discards factory results in exception tests while preserving their evaluation.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/core/mcp/validation.h Outdated
Comment thread src/core/mcp/validation.h Outdated
@HarshPopat23
HarshPopat23 force-pushed the perf/mcp-reduce-temporary-allocations branch from aa5d198 to ef8c35d Compare October 16, 2026 07:05
@HarshPopat23 HarshPopat23 changed the title Avoid temporary allocations in MCP validation and parameter parsing Avoid temporary allocations in MCP validation Oct 16, 2026
@HarshPopat23

Copy link
Copy Markdown
Contributor Author

augment review

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

View guided diff | Re-trigger cubic

@HarshPopat23
HarshPopat23 force-pushed the perf/mcp-reduce-temporary-allocations branch from ef8c35d to c704323 Compare October 16, 2026 08:04

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread src/core/mcp/include/sourcemeta/core/mcp_logging.h Outdated
@HarshPopat23 HarshPopat23 changed the title Avoid temporary allocations in MCP validation Improve MCP validation efficiency and annotate public APIs with nodiscard Oct 16, 2026
@HarshPopat23
HarshPopat23 force-pushed the perf/mcp-reduce-temporary-allocations branch from aea02e4 to 205435d Compare October 16, 2026 08:32
@HarshPopat23

Copy link
Copy Markdown
Contributor Author

augment review

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread src/core/mcp/include/sourcemeta/core/mcp_error.h
Comment thread src/core/mcp/include/sourcemeta/core/mcp_results.h
@HarshPopat23
HarshPopat23 force-pushed the perf/mcp-reduce-temporary-allocations branch from 205435d to d10e9ce Compare October 16, 2026 09:01
@HarshPopat23

Copy link
Copy Markdown
Contributor Author

augment review

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

…card

- Replace ad-hoc array of strings validation loop with is_array_of_strings() in validation.h.
- Avoid unnecessary string allocations when dispatching message and content types.
- Annotate public MCP functions across protocol, headers, results, capabilities, error, and logging with [[nodiscard]].
- Update test helpers to cast discarded results to void.

Signed-off-by: HarshPopat23 <musichk61@gmail.com>
@HarshPopat23
HarshPopat23 force-pushed the perf/mcp-reduce-temporary-allocations branch from d10e9ce to 8f7d7f2 Compare October 16, 2026 09:04
@HarshPopat23

Copy link
Copy Markdown
Contributor Author

augment review

@HarshPopat23 HarshPopat23 reopened this Oct 16, 2026

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