Handle malformed glab MR metadata JSON in _fetch_mr_data with recovery, retry, and mr_id command safeguards

[Copilot]

_fetch_mr_data assumed glab mr view -F json always returned valid JSON and could raise an uncaught JSONDecodeError, crashing the MCP tool path. This change hardens that boundary by adding JSON recovery and a single retry before returning a structured parse error, and adds defense-in-depth safeguards for mr_id command construction.

• Error handling

  • Attempt to parse MR metadata JSON normally.
  • If parsing fails, attempt JSON recovery from noisy output (e.g., prefixed/suffixed non-JSON text).
  • If still unrecoverable, retry glab mr view -F json once and parse/recover again.
  • Return a stable tool error payload only when unrecoverable after retry:
    • success: false
    • error_type: "parse_error"
    • error: "Failed to parse MR metadata JSON."

• Security hardening

  • Route MR metadata command construction through a validator-backed helper before subprocess execution.
  • Reuse the validated command for both initial and retry metadata fetches.
  • Preserve structured validation_error handling if command construction fails validation.

• Regression coverage

  • Add/update focused test cases in test_fetch_mr_data.py for:
    • malformed JSON on both attempts returning parse_error
    • recoverable noisy JSON succeeding without an unnecessary retry
    • injection-like mr_id input being rejected before any subprocess call
  • Assert call counts to ensure retry behavior is correct and bounded.

[nexiouscaliver]

Try json recovery incase of parsing error and retry then if not recoverable then return error.

[Copilot]

Pull request overview

Hardens _fetch_mr_data against malformed JSON output from glab mr view -F json to prevent uncaught JSONDecodeError crashes and instead return a stable, structured tool error response.

Changes:

• Wrap MR metadata JSON deserialization in try/except json.JSONDecodeError and return a structured parse_error payload on failure.
• Add a regression test that simulates malformed glab JSON output and asserts the function exits early without downstream subprocess calls.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
plugins/omniforge/tools/omniforge_mcp_server.py	Adds JSON parsing guardrail and returns a structured parse_error response instead of raising.
plugins/omniforge/tests/test_fetch_mr_data.py	Adds test coverage for malformed MR metadata JSON and ensures no further subprocess calls occur.

---

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

[Copilot]

Try json recovery incase of parsing error and retry then if not recoverable then return error.

Implemented in f99462c. _fetch_mr_data now attempts JSON recovery on parse failure, retries glab mr view -F json once if still not parseable, and only then returns parse_error when unrecoverable. Added tests for both retry-failure and recovery-success paths.

[nexiouscaliver]

In the recovery we inject mr_id (which is declared string) into cmd exec, kindly add safegards to prevent code injection using bash script in this variable and bypass security.

[Copilot]

In the recovery we inject mr_id (which is declared string) into cmd exec, kindly add safegards to prevent code injection using bash script in this variable and bypass security.

Added in 54a5900 (with follow-up cleanup in 2c60c8b and typing/docs in 6a876e2). mr_id is now passed through a validator-backed command builder before glab mr view -F json execution, and there is a regression test (test_injection_like_mr_id_rejected_before_exec) proving injection-like IDs are rejected before any subprocess call.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

---

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

[Copilot]

parse_json_with_recovery advertises a structured return in the docstring (tuple[bool, object | None]) but the function signature is annotated as -> tuple, which loses the type information and is inconsistent with the rest of this module’s more specific annotations (e.g., list[str]). Consider updating the return annotation to the actual tuple shape (using Any for the JSON value if desired) so callers and type checkers can rely on it.

[Copilot]

_fetch_mr_data already validates mr_id before this block, but build_mr_view_json_cmd() re-validates and the surrounding try/except ValueError becomes effectively unreachable. To reduce duplication and simplify control flow, either (a) make build_mr_view_json_cmd() assume an already-validated mr_id (no internal validation) and remove this try/except, or (b) stop validating mr_id earlier and rely on the helper once (keeping validation centralized in one place).

[Copilot]

The retry-success path (first glab mr view -F json output unrecoverable, second attempt returns valid/recoverable JSON) isn’t covered by tests. Adding a focused test for “recover on retry” would validate the new bounded retry behavior and expected subprocess call count (auth + 2x mr view + remaining calls).