Skip to content

[Repo Assist] fix(rust-guard): remove dead .or_else() fallback in notifications arm#8935

Merged
lpcox merged 2 commits into
mainfrom
repo-assist/fix-rust-guard-notifications-deadcode-8921-c74174217471e7bf
Jul 8, 2026
Merged

[Repo Assist] fix(rust-guard): remove dead .or_else() fallback in notifications arm#8935
lpcox merged 2 commits into
mainfrom
repo-assist/fix-rust-guard-notifications-deadcode-8921-c74174217471e7bf

Conversation

@github-actions

@github-actions github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

🤖 This is an automated pull request from Repo Assist.

Closes #8921

Root Cause

In guards/github-guard/rust-guard/src/labels/response_items.rs, the list_notifications | get_notification_details arm had dead code:

let items = actual_response.as_array().or_else(|| response.as_array());

The .or_else(|| response.as_array()) fallback is always a no-op because extract_mcp_response (which produces actual_response) returns either:

  • Cow::Owned(parsed)parsed was successfully deserialized from content[0].text, so actual_response.as_array() is already Some(_); the fallback is unreachable.
  • Cow::Borrowed(response)actual_response is response, so .or_else(|| response.as_array()) checks the identical pointer twice.

This pattern is absent in every other arm of the same match block (they all call collect_items_simple(&actual_response) or extract_items_slice(&actual_response, ...) directly), making the notifications arm inconsistently defensive without benefit.

Fix

Remove the .or_else(...) chain, leaving the simpler:

let items = actual_response.as_array();

No behaviour change — the removed path was never executed.

Tests Added

The notifications arm previously had zero test coverage. Three tests are added:

Test Covers
list_notifications_labels_each_item_as_private private_user_label() secrecy assignment for each notification
list_notifications_description_includes_id "notification:{id}" description format
get_notification_details_empty_array_returns_empty empty array input → empty result

Test Status

All 576 Rust tests pass (up from 573 before adding the new tests):

test result: ok. 576 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

⚠️ Go unit tests blocked by network restrictions in this CI environment (pre-existing infrastructure issue — proxy.golang.org is inaccessible). This is a Rust-only change.


Generated by Repo Assist

Warning

Firewall blocked 2 domains

The following domains were blocked by the firewall during workflow execution:

  • awmgmcpg
  • proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"
    - "proxy.golang.org"

See Network Configuration for more information.

Generated by Repo Assist · 716.4 AIC · ⊞ 10.8K ·
Comment /repo-assist to run again

Add this agentic workflow to your repo

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@851905c06e905bf362a9f6cc54f912e3df747d55

The `.or_else(|| response.as_array())` on line 395 of response_items.rs
was always a no-op because `extract_mcp_response` returns either:
- `Cow::Owned(parsed)`: parsing succeeded, so `actual_response.as_array()`
  is already the result — the fallback is unreachable.
- `Cow::Borrowed(response)`: `actual_response` IS `response`, so the
  fallback checks the identical value twice.

This removes the dead fallback and makes the notifications arm consistent
with every other arm in the file (all of which use `actual_response`
directly, without a `response` fallback).

Adds three tests that previously had zero coverage for this arm:
- list_notifications_labels_each_item_as_private: verifies the
  private_user_label() secrecy assignment for each notification.
- list_notifications_description_includes_id: verifies the
  "notification:{id}" description format.
- get_notification_details_empty_array_returns_empty: verifies an
  empty array input produces no labeled items.

All 576 Rust tests pass after this change (was 573 before adding tests).

Closes #8921

🤖 Generated by Repo Assist

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox lpcox marked this pull request as ready for review July 8, 2026 13:45
Copilot AI review requested due to automatic review settings July 8, 2026 13:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR simplifies Rust Guard item-labeling for GitHub notifications by removing an unreachable .or_else() fallback and adds targeted unit tests to cover the previously untested notifications arm in label_response_items.

Changes:

  • Remove dead .or_else(|| response.as_array()) fallback in the notifications match arm.
  • Add unit tests covering notification secrecy labeling, description formatting, and empty-array behavior.
Show a summary per file
File Description
guards/github-guard/rust-guard/src/labels/response_items.rs Removes unreachable fallback logic in notifications handling and adds new tests for notification labeling behavior.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread guards/github-guard/rust-guard/src/labels/response_items.rs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[rust-guard] Rust Guard: Remove dead .or_else() fallback in notifications arm + add coverage tests

2 participants