Fix/failure false if signature is fetched

[mmilanta]

No description provided.

[qodo-merge-etso]

Review Summary by Qodo

Fix false failure flag when signature is fetched from server

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Fix signature fetch logic to prevent false failures
  - Only update signature if server received one
  - Reset error failure flag when signature is fetched
• Add test fixtures for OAuth catalog configuration
• Add end-to-end test for catalog server scanning

Diagram

flowchart LR
  A["Server signature missing locally"] -->|"Check if remote has signature"| B["Remote signature exists?"]
  B -->|"Yes"| C["Update local signature"]
  C --> D["Reset error is_failure flag"]
  D --> E["Success"]
  B -->|"No"| F["Skip update"]

Loading

File Changes

1. src/agent_scan/verify_api.py 🐞 Bug fix +3/-1

Fix signature fetch failure flag logic

• Enhanced signature update condition to check if remote signature exists
• Added logic to reset error failure flag when signature is successfully fetched
• Prevents false failure states when servers have signatures but initial errors

src/agent_scan/verify_api.py

---

2. tests/conftest.py 🧪 Tests +21/-0

Add OAuth catalog test fixtures

• Added fixture for OAuth catalog configuration file
• Added fixture for remote server with OAuth in catalog config
• Provides test configuration with MCP server settings

tests/conftest.py

---

3. tests/e2e/test_scan.py 🧪 Tests +21/-0

Add catalog server scanning end-to-end test

• Added end-to-end test for scanning servers in catalog
• Validates signature is fetched from remote server
• Verifies error exists but is_failure flag is False
• Tests catalog server scanning with OAuth configuration

tests/e2e/test_scan.py

---

[qodo-merge-etso]

Code Review by Qodo

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again

ⓘ The new review experience is currently in Beta. Learn more

[qodo-merge-etso]

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: test (ubuntu-latest)

Failed stage: Run tests [❌]

Failed test name: tests/e2e/test_scan.py::TestFullScanFlow::test_scan[tests/mcp_servers/configs_files/math_config.json-server_names1-binary]

Failure summary: The GitHub Action failed because the CI test suite had 2 failing end-to-end tests in tests/e2e/test_scan.py. - tests/e2e/test_scan.py:127 asserts that the set of issue codes returned by the scan --json output matches one of the allowed_issue_sets. - For both math_config.json and all_config.json scans, the actual issue_set was {'W001'}, but the tests expected sets that also include W003 (and sometimes TF001/TF002), so the assertion failed. - As a result, make ci exited with error (Makefile:28: ci), and the job ended with a non-zero exit code (Process completed with exit code 2).

Relevant error logs: 1: ##[group]Runner Image Provisioner 2: Hosted Compute Agent ... 431: tests/e2e/test_inspect.py::TestInspect::test_direct_scan[sse-binary] PASSED [ 6%] 432: tests/e2e/test_inspect.py::TestInspect::test_direct_scan_stdio_servers[npm_latest-binary] PASSED [ 6%] 433: tests/e2e/test_inspect.py::TestInspect::test_direct_scan_stdio_servers[npm_versioned-binary] PASSED [ 7%] 434: tests/e2e/test_inspect.py::TestInspect::test_direct_scan_stdio_servers[pypi_latest-binary] PASSED [ 7%] 435: tests/e2e/test_inspect.py::TestInspect::test_direct_scan_stdio_servers[pypi_versioned-binary] PASSED [ 8%] 436: tests/e2e/test_inspect.py::TestInspect::test_direct_scan_stdio_servers[oci-binary] PASSED [ 8%] 437: tests/e2e/test_inspect.py::TestInspect::test_vscode_settings_no_mcp[binary] PASSED [ 9%] 438: tests/e2e/test_scan.py::TestFullScanFlow::test_basic[claudestyle_config_file-binary] PASSED [ 9%] 439: tests/e2e/test_scan.py::TestFullScanFlow::test_basic[vscode_mcp_config_file-binary] PASSED [ 10%] 440: tests/e2e/test_scan.py::TestFullScanFlow::test_basic[vscode_config_file-binary] PASSED [ 10%] 441: tests/e2e/test_scan.py::TestFullScanFlow::test_basic[streamable_http_transport_config_file-binary] PASSED [ 11%] 442: tests/e2e/test_scan.py::TestFullScanFlow::test_basic[sse_transport_config_file-binary] PASSED [ 11%] 443: tests/e2e/test_scan.py::TestFullScanFlow::test_scan_sse_http[streamable_http_transport_config_file-binary] PASSED [ 12%] 444: tests/e2e/test_scan.py::TestFullScanFlow::test_scan_sse_http[sse_transport_config_file-binary] PASSED [ 12%] 445: tests/e2e/test_scan.py::TestFullScanFlow::test_scan[tests/mcp_servers/configs_files/weather_config.json-server_names0-binary] PASSED [ 13%] 446: tests/e2e/test_scan.py::TestFullScanFlow::test_scan[tests/mcp_servers/configs_files/math_config.json-server_names1-binary] FAILED [ 13%] 447: tests/e2e/test_scan.py::TestFullScanFlow::test_scan[tests/mcp_servers/configs_files/all_config.json-server_names2-binary] FAILED [ 13%] 448: tests/e2e/test_scan.py::TestFullScanFlow::test_scan_skills_without_flag[skills_parent_dir-binary] PASSED [ 14%] ... 453: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers[options_before_first_server_ignored] PASSED [ 16%] 454: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers[no_control_servers] PASSED [ 17%] 455: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers[control_server_without_url] PASSED [ 17%] 456: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers[url_starts_with_dash] PASSED [ 18%] 457: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers[with_other_cli_args] PASSED [ 18%] 458: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers[single_server_with_multiple_headers] PASSED [ 19%] 459: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers_missing_identifier[single_server_no_identifier] PASSED [ 19%] 460: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers_missing_identifier[single_server_headers_only_no_identifier] PASSED [ 20%] 461: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers_missing_identifier[multiple_servers_one_missing_identifier] PASSED [ 20%] 462: tests/unit/test_cli_parsing.py::TestControlServerParsing::test_parse_control_servers_missing_identifier[options_only_apply_to_preceding_server] PASSED [ 20%] 463: tests/unit/test_cli_parsing.py::TestCLIArgumentParsing::test_scan_with_multiple_control_servers_parses_correctly PASSED [ 21%] 464: tests/unit/test_cli_parsing.py::TestControlServerHeaderParsing::test_parse_headers_single_header PASSED [ 21%] 465: tests/unit/test_cli_parsing.py::TestControlServerHeaderParsing::test_parse_headers_multiple_headers PASSED [ 22%] 466: tests/unit/test_cli_parsing.py::TestControlServerHeaderParsing::test_parse_headers_none_input PASSED [ 22%] 467: tests/unit/test_cli_parsing.py::TestControlServerHeaderParsing::test_parse_headers_empty_list PASSED [ 23%] 468: tests/unit/test_cli_parsing.py::TestControlServerHeaderParsing::test_parse_headers_invalid_format_raises_error PASSED [ 23%] 469: tests/unit/test_cli_parsing.py::TestControlServerUploadIntegration::test_control_servers_passed_to_pipeline PASSED [ 24%] ... 471: tests/unit/test_cli_parsing.py::TestControlServerUploadIntegration::test_skip_ssl_verify_passed_to_pipeline PASSED [ 25%] 472: tests/unit/test_cli_parsing.py::TestJSONOutput::test_json_output_suppresses_stdout_during_scan PASSED [ 25%] 473: tests/unit/test_cli_parsing.py::TestJSONOutput::test_json_output_only_contains_json PASSED [ 26%] 474: tests/unit/test_config_scan.py::test_scan_mcp_config[claudestyle_config_file] PASSED [ 26%] 475: tests/unit/test_config_scan.py::test_scan_mcp_config[vscode_mcp_config_file] PASSED [ 26%] 476: tests/unit/test_config_scan.py::test_scan_mcp_config[vscode_config_file] PASSED [ 27%] 477: tests/unit/test_config_scan.py::test_scan_mcp_config[vscode_settings_file_with_empty_mcp] PASSED [ 27%] 478: tests/unit/test_config_scan.py::test_scan_mcp_config[vscode_settings_file_without_mcp] PASSED [ 28%] 479: tests/unit/test_config_scan.py::test_check_server_mocked PASSED [ 28%] 480: tests/unit/test_config_scan.py::test_math_server PASSED [ 29%] 481: tests/unit/test_config_scan.py::test_all_server PASSED [ 29%] 482: tests/unit/test_config_scan.py::test_weather_server PASSED [ 30%] 483: tests/unit/test_config_scan.py::test_vscode_settings_file_without_mcp PASSED [ 30%] 484: tests/unit/test_config_scan.py::test_vscode_settings_file_with_empty_mcp PASSED [ 31%] 485: tests/unit/test_control_server.py::test_upload_payload_excludes_hostname_and_username PASSED [ 31%] 486: tests/unit/test_control_server.py::test_upload_includes_scan_error_in_payload PASSED [ 32%] 487: tests/unit/test_control_server.py::test_upload_file_not_found_error_in_payload PASSED [ 32%] 488: tests/unit/test_control_server.py::test_upload_parse_error_in_payload PASSED [ 33%] 489: tests/unit/test_control_server.py::test_upload_server_http_error_in_payload PASSED [ 33%] 490: tests/unit/test_control_server.py::test_upload_server_startup_error_in_payload PASSED [ 33%] 491: tests/unit/test_control_server.py::test_upload_retries_on_network_error PASSED [ 34%] 492: tests/unit/test_control_server.py::test_upload_retries_on_server_error PASSED [ 34%] 493: tests/unit/test_control_server.py::test_upload_does_not_retry_on_client_error PASSED [ 35%] 494: tests/unit/test_control_server.py::test_upload_succeeds_on_second_attempt PASSED [ 35%] 495: tests/unit/test_control_server.py::test_upload_custom_max_retries PASSED [ 36%] 496: tests/unit/test_control_server.py::test_upload_exponential_backoff PASSED [ 36%] 497: tests/unit/test_control_server.py::test_upload_does_not_retry_on_unexpected_error PASSED [ 37%] 498: tests/unit/test_control_server.py::test_upload_unknown_mcp_config_error_in_payload PASSED [ 37%] 499: tests/unit/test_entity_to_tool.py::test_entity_to_tool[tests/mcp_servers/signatures/math_server_signature.json] PASSED [ 38%] ... 576: tests/unit/test_utils.py::TestRebalanceCommandArgsWithSpacesInPath::test_full_command_is_path_with_spaces_no_args PASSED [ 73%] 577: tests/unit/test_utils.py::test_calculate_distance PASSED [ 74%] 578: tests/unit/test_utils.py::TestSuppressStdout::test_suppress_stdout_suppresses_print PASSED [ 74%] 579: tests/unit/test_utils.py::TestSuppressStdout::test_suppress_stdout_restores_stdout_after_context PASSED [ 75%] 580: tests/unit/test_utils.py::TestSuppressStdout::test_suppress_stdout_works_with_multiple_prints PASSED [ 75%] 581: tests/unit/test_verify_api.py::TestProxySupport::test_analyze_machine_honors_http_proxy_env PASSED [ 76%] 582: tests/unit/test_verify_api.py::TestProxySupport::test_analyze_machine_honors_https_proxy_env PASSED [ 76%] 583: tests/unit/test_verify_api.py::TestProxySupport::test_analyze_machine_works_without_proxy PASSED [ 77%] 584: tests/unit/test_verify_api.py::TestProxySupport::test_analyze_machine_with_skip_ssl_verify_and_proxy PASSED [ 77%] 585: tests/unit/test_verify_api.py::TestProxySupport::test_setup_tcp_connector_with_ssl_verify PASSED [ 78%] 586: tests/unit/test_verify_api.py::TestProxySupport::test_setup_tcp_connector_without_ssl_verify PASSED [ 78%] 587: tests/unit/test_verify_api.py::TestAnalyzeMachineRetries::test_analyze_machine_retries_on_timeout PASSED [ 79%] 588: tests/unit/test_verify_api.py::TestAnalyzeMachineHeaders::test_analyze_machine_includes_additional_headers PASSED [ 79%] 589: tests/unit/test_verify_api.py::TestAnalyzeMachineScanMetadata::test_analyze_machine_includes_scan_metadata_when_scan_context_provided PASSED [ 80%] 590: tests/unit/test_verify_api.py::TestAnalyzeMachineScanMetadata::test_analyze_machine_omits_scan_metadata_when_scan_context_not_provided PASSED [ 80%] 591: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[400] PASSED [ 80%] 592: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[401] PASSED [ 81%] 593: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[403] PASSED [ 81%] 594: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[413] PASSED [ 82%] 595: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[422] PASSED [ 82%] 596: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[429] PASSED [ 83%] 597: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[500] PASSED [ 83%] 598: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[502] PASSED [ 84%] 599: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[503] PASSED [ 84%] 600: tests/unit/test_verify_api.py::TestAnalyzeMachineHttpErrors::test_analyze_machine_http_error_responses[504] PASSED [ 85%] 601: tests/v4compatibility/test_inspect.py::test_get_mcp_config_per_client[client0-valid-True-True] PASSED [ 85%] ... 618: tests/v4compatibility/test_inspect.py::test_inspect_skill[theme-factory-skill_server2] PASSED [ 93%] 619: tests/v4compatibility/test_inspect.py::test_inspect_skill[malicious-skill-skill_server3] PASSED [ 93%] 620: tests/v4compatibility/test_inspect.py::test_inspect_skill[brand-guidelines-skill_server4] PASSED [ 94%] 621: tests/v4compatibility/test_inspect.py::test_inspect_skill[algorithmic-art-skill_server5] PASSED [ 94%] 622: tests/v4compatibility/test_inspect.py::test_inspect_skill[pdf-skill_server6] PASSED [ 95%] 623: tests/v4compatibility/test_inspect.py::test_inspect_skill[web-artifacts-builder-skill_server7] PASSED [ 95%] 624: tests/v4compatibility/test_inspect.py::test_inspect_skill[skill-creator-skill_server8] PASSED [ 96%] 625: tests/v4compatibility/test_inspect.py::test_inspect_skill[slack-gif-creator-skill_server9] PASSED [ 96%] 626: tests/v4compatibility/test_inspect.py::test_inspect_skill[webapp-testing-skill_server10] PASSED [ 97%] 627: tests/v4compatibility/test_inspect.py::test_inspect_skill[xlsx-skill_server11] PASSED [ 97%] 628: tests/v4compatibility/test_inspect.py::test_inspect_skill[canvas-design-skill_server12] PASSED [ 98%] 629: tests/v4compatibility/test_inspect.py::test_inspect_skill[mcp-builder-skill_server13] PASSED [ 98%] 630: tests/v4compatibility/test_inspect.py::test_inspect_skill[frontend-design-skill_server14] PASSED [ 99%] 631: tests/v4compatibility/test_inspect.py::test_inspect_skill[docx-skill_server15] PASSED [ 99%] 632: tests/v4compatibility/test_inspect.py::test_inspect_skill[doc-coauthoring-skill_server16] PASSED [100%] 633: =================================== FAILURES =================================== 634: _ TestFullScanFlow.test_scan[tests/mcp_servers/configs_files/math_config.json-server_names1-binary] _ ... 646: ], 647: ) 648: def test_scan(self, agent_scan_cmd, path, server_names): 649: result = subprocess.run( 650: [ 651: *agent_scan_cmd, 652: "scan", 653: "--json", 654: path, 655: "--analysis-url", 656: "https://api.snyk.io/hidden/mcp-scan/analysis-machine?version=2025-09-07", 657: ], 658: capture_output=True, 659: text=True, 660: ) 661: assert result.returncode == 0, f"Command failed with error: {result.stderr}" 662: output = json.loads(result.stdout) 663: for server in output[path]["servers"]: 664: server["signature"]["metadata"]["serverInfo"]["version"] = ( 665: "mcp_version" # swap actual version with placeholder 666: ) 667: with open(f"tests/mcp_servers/signatures/{server['name'].lower()}_server_signature.json") as f: 668: assert server["signature"] == json.load(f), f"Signature mismatch for {server['name']} server" 669: assert len(output) == 1, "Output should contain exactly one entry for the config file" 670: path = next(iter(output.keys())) 671: errors = output[path]["error"] 672: assert errors is None, f"Error should not be present, found: {errors}" 673: issues = output[path]["issues"] 674: issue_set = {issue["code"] for issue in issues} 675: if set(server_names) == {"Weather", "Math"}: 676: allowed_issue_sets = [{"W001", "W003", "TF001", "TF002"}, {"W001", "W003", "TF002"}, {"W001", "W003"}] 677: elif set(server_names) == {"Weather"}: 678: allowed_issue_sets = [{"W003"}, {"W003", "TF001"}, set()] 679: elif set(server_names) == {"Math"}: 680: allowed_issue_sets = [{"W001", "W003"}, {"W001", "W003", "TF002"}] 681: else: 682: raise ValueError(f"Invalid server names: {server_names}") 683: # call list for better error message 684: > assert any(issue_set == ais for ais in allowed_issue_sets), ( 685: f"Issues codes {issue_set} do not match expected values {allowed_issue_sets}" 686: ) 687: E AssertionError: Issues codes {'W001'} do not match expected values [{'W003', 'W001'}, {'TF002', 'W003', 'W001'}] 688: E assert False 689: E + where False = any(<generator object TestFullScanFlow.test_scan.<locals>.<genexpr> at 0x7fa8a0c8a8e0>) 690: tests/e2e/test_scan.py:127: AssertionError 691: _ TestFullScanFlow.test_scan[tests/mcp_servers/configs_files/all_config.json-server_names2-binary] _ ... 703: ], 704: ) 705: def test_scan(self, agent_scan_cmd, path, server_names): 706: result = subprocess.run( 707: [ 708: *agent_scan_cmd, 709: "scan", 710: "--json", 711: path, 712: "--analysis-url", 713: "https://api.snyk.io/hidden/mcp-scan/analysis-machine?version=2025-09-07", 714: ], 715: capture_output=True, 716: text=True, 717: ) 718: assert result.returncode == 0, f"Command failed with error: {result.stderr}" 719: output = json.loads(result.stdout) 720: for server in output[path]["servers"]: 721: server["signature"]["metadata"]["serverInfo"]["version"] = ( 722: "mcp_version" # swap actual version with placeholder 723: ) 724: with open(f"tests/mcp_servers/signatures/{server['name'].lower()}_server_signature.json") as f: 725: assert server["signature"] == json.load(f), f"Signature mismatch for {server['name']} server" 726: assert len(output) == 1, "Output should contain exactly one entry for the config file" 727: path = next(iter(output.keys())) 728: errors = output[path]["error"] 729: assert errors is None, f"Error should not be present, found: {errors}" 730: issues = output[path]["issues"] 731: issue_set = {issue["code"] for issue in issues} 732: if set(server_names) == {"Weather", "Math"}: 733: allowed_issue_sets = [{"W001", "W003", "TF001", "TF002"}, {"W001", "W003", "TF002"}, {"W001", "W003"}] 734: elif set(server_names) == {"Weather"}: 735: allowed_issue_sets = [{"W003"}, {"W003", "TF001"}, set()] 736: elif set(server_names) == {"Math"}: 737: allowed_issue_sets = [{"W001", "W003"}, {"W001", "W003", "TF002"}] 738: else: 739: raise ValueError(f"Invalid server names: {server_names}") 740: # call list for better error message 741: > assert any(issue_set == ais for ais in allowed_issue_sets), ( 742: f"Issues codes {issue_set} do not match expected values {allowed_issue_sets}" 743: ) 744: E AssertionError: Issues codes {'W001'} do not match expected values [{'TF002', 'W003', 'W001', 'TF001'}, {'TF002', 'W003', 'W001'}, {'W003', 'W001'}] 745: E assert False 746: E + where False = any(<generator object TestFullScanFlow.test_scan.<locals>.<genexpr> at 0x7fa8a0c8b920>) 747: tests/e2e/test_scan.py:127: AssertionError 748: =============================== warnings summary =============================== 749: tests/unit/test_control_server.py: 25 warnings 750: tests/unit/test_verify_api.py: 20 warnings 751: /home/runner/work/agent-scan/agent-scan/.venv/lib/python3.13/site-packages/aiohttp/connector.py:993: DeprecationWarning: enable_cleanup_closed ignored because https://github.com/python/cpython/pull/118960 is fixed in Python version sys.version_info(major=3, minor=13, micro=12, releaselevel='final', serial=0) 752: super().__init__( 753: -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html 754: =========================== short test summary info ============================ 755: FAILED tests/e2e/test_scan.py::TestFullScanFlow::test_scan[tests/mcp_servers/configs_files/math_config.json-server_names1-binary] - AssertionError: Issues codes {'W001'} do not match expected values [{'W003', 'W001'}, {'TF002', 'W003', 'W001'}] 756: assert False 757: + where False = any(<generator object TestFullScanFlow.test_scan.<locals>.<genexpr> at 0x7fa8a0c8a8e0>) 758: FAILED tests/e2e/test_scan.py::TestFullScanFlow::test_scan[tests/mcp_servers/configs_files/all_config.json-server_names2-binary] - AssertionError: Issues codes {'W001'} do not match expected values [{'TF002', 'W003', 'W001', 'TF001'}, {'TF002', 'W003', 'W001'}, {'W003', 'W001'}] 759: assert False 760: + where False = any(<generator object TestFullScanFlow.test_scan.<locals>.<genexpr> at 0x7fa8a0c8b920>) 761: ===== 2 failed, 213 passed, 34 deselected, 45 warnings in 65.36s (0:01:05) ===== 762: make: *** [Makefile:28: ci] Error 1 763: ##[error]Process completed with exit code 2. 764: Post job cleanup.

[hemang-snyk]

Should we attach some metadata with the error here or the server so that we know that we failed to start the server but it matched something in the catalog? It can get confusing in the platform otherwise all fields look like the server failed to start but is_failure is set to False.