Skip to content

fix: address review feedback on diagnostics multi-device discovery - #4

Merged
mad201802 merged 2 commits into
fix/diagnostics_discoveryfrom
copilot/sub-pr-3
Mar 3, 2026
Merged

mad201802 merged 2 commits into
fix/diagnostics_discoveryfrom
copilot/sub-pr-3

Conversation

Copilot AI commented Mar 3, 2026 •

Copy link
Copy Markdown
Contributor

Addresses four code review issues from the diagnostics multi-device discovery PR: a silent failure path in desktop_peer, a use-after-free in diag_scanner, duplicated request logic in runtime.hpp, and missing test coverage for the explicit-target request overload.

Changes

  • desktop_peer.cpp: Check enable_diagnostics() return value; log to stderr on failure instead of silently continuing without diagnostics.

  • diag_scanner.cpp: Change devices from std::vector to std::deque. vector::push_back can reallocate, invalidating &dev pointers stored as callback context for in-flight async requests. deque::push_back guarantees pointer stability for existing elements.

  • runtime.hpp: Extract private request_impl(const Addr& target, ...) helper; both public request overloads delegate to it, eliminating duplicated allocate/build/send logic.

// Both overloads now share one implementation path
std::optional<uint32_t> request(uint16_t service_id, ...) {
    Addr provider{};
    if (!sd_.get_provider_address(service_id, provider)) return std::nullopt;
    return request_impl(provider, service_id, ...);
}

std::optional<uint32_t> request(const Addr& target, ...) {
    return request_impl(target, service_id, ...);
}
  • test_runtime.cpp: Add two tests for the explicit-target request overload — ClientRequest_ExplicitTarget_SendsWithoutSD (sends to target when SD has no provider) and ClientRequest_ExplicitTarget_ResponseCompletesCallback (response from target correctly fires callback).

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…e storage, request_impl refactor, new tests

Co-authored-by: mad201802 <45396189+mad201802@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix diagnostics multi device discovery fix: address review feedback on diagnostics multi-device discovery Mar 3, 2026
@mad201802
mad201802 marked this pull request as ready for review March 3, 2026 23:21
@mad201802
mad201802 merged commit 21af96a into fix/diagnostics_discovery Mar 3, 2026
mad201802 added a commit that referenced this pull request Mar 3, 2026
* feat: enhance diagnostics discovery with direct device querying and dashboard updates

* fix: address review feedback on diagnostics multi-device discovery (#4)

* Initial plan

* fix: address review feedback - check enable_diagnostics return, stable storage, request_impl refactor, new tests

Co-authored-by: mad201802 <45396189+mad201802@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: mad201802 <45396189+mad201802@users.noreply.github.com>

---------

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
@mad201802
mad201802 deleted the copilot/sub-pr-3 branch March 17, 2026 19:58
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.

2 participants