Skip to content

fix getting a2a config #10942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 30, 2025
Merged

fix getting a2a config #10942

merged 2 commits into from
May 30, 2025

Conversation

hamishfagg
Copy link
Contributor

Description

Please include a summary of the change and the issue it solves.

Fixes #issue_number

Type of change

(Please delete options that are not relevant)

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ⚡ New feature (non-breaking change which adds functionality)
  • 📢 Breaking change (fix or feature that would cause existing functionality not to work as expected)
  • 📄 This change requires a documentation update

Verification Process

To ensure the changes are working as expected:

  • Test Location: Specify the URL or path for testing.
  • Verification Steps: Outline the steps or queries needed to validate the change. Include any data, configurations, or actions required to reproduce or see the new functionality.

Additional Media:

  • I have attached a brief loom video or screenshots showcasing the new functionality or change.

Checklist:

  • My code follows the style guidelines(PEP 8) of MindsDB.
  • I have appropriately commented on my code, especially in complex areas.
  • Necessary documentation updates are either made or tracked in issues.
  • Relevant unit and integration tests are updated or added.

Copy link

Review Summary

Skipped posting 6 drafted comments based on your review threshold. Feel free to update them here.

Draft Comments
mindsdb/api/a2a/run_a2a.py:60-60
`config.get("api", {})` may return a non-dict if the "api" key is missing or not a dict, causing `.get("a2a", {})` to raise `AttributeError` at runtime.

Scores:

  • Production Impact: 4
  • Fix Specificity: 5
  • Urgency Impact: 3
  • Total Score: 12

Reason for filtering: The total score does not exceed the required threshold of 13 for inclusion under the current filtering rules.

Analysis: The bug could cause runtime errors if config['api'] is not a dict, which is a significant issue (score 4). The fix is clear and directly applicable (score 5). However, the urgency is medium since it will not always break immediately (score 3). The total score is 12, which is below the required threshold, so the comment should be removed.

mindsdb/api/a2a/run_a2a.py:8-10
Repeated logging setup in `main` and module-level can be refactored to avoid redundancy and improve maintainability.

Scores:

  • Production Impact: 1
  • Fix Specificity: 5
  • Urgency Impact: 2
  • Total Score: 8

Reason for filtering: The comment addresses code maintainability and redundancy, not a critical bug or production issue. The impact on production is minimal, the fix is clear but not urgent, and the urgency is low. The total score does not meet the required threshold.

Analysis: This is a maintainability suggestion with minimal production impact and low urgency. The fix is specific, but the overall importance is not high enough to include under strict filtering.

mindsdb/api/a2a/run_a2a.py:79-81
The import of `traceback` inside the exception block in `main` is inefficient; move import to the top-level to avoid repeated imports.

Scores:

  • Production Impact: 1
  • Fix Specificity: 5
  • Urgency Impact: 2
  • Total Score: 8

Reason for filtering: The comment addresses a minor performance inefficiency (importing inside a function), which has minimal production impact and low urgency. The fix is clear but not critical. The total score does not meet the strict threshold.

Analysis: Importing 'traceback' inside an exception block is a minor inefficiency, not a production risk. The fix is straightforward, but the urgency and impact are low, resulting in a low total score.

mindsdb/api/a2a/run_a2a.py:69-69
Sensitive configuration data in `a2a_config` is logged in plaintext, risking exposure of secrets or credentials if logs are accessed.

Scores:

  • Production Impact: 3
  • Fix Specificity: 5
  • Urgency Impact: 4
  • Total Score: 12

Reason for filtering: The total score is 12, which does not meet the required threshold of greater than 13 for inclusion.

Analysis: Logging sensitive configuration data is a security risk but does not directly cause production crashes or failures, so the production impact is moderate. The fix is very specific and directly applicable. The urgency is high due to the security implications, but not critical for immediate system impairment. The total score does not meet the strict filtering threshold.

mindsdb/utilities/starters.py:65-65
`config.get("api", {}).get("a2a", {})` may return `{}` if `api` or `a2a` is missing, which could cause `main` to receive an empty config and fail if required keys are missing.

Scores:

  • Production Impact: 4
  • Fix Specificity: 5
  • Urgency Impact: 3
  • Total Score: 12

Reason for filtering: The total score does not exceed the required threshold of 13 for inclusion under the current filtering rules.

Analysis: The bug could cause failures if required config keys are missing, which is a significant issue (impact 4). The fix is clear and directly applicable (specificity 5). However, the urgency is medium since the failure depends on missing config, not an immediate crash (urgency 3). The total score is 12, which is below the threshold, so the comment should be removed.

mindsdb/utilities/starters.py:3-3
Unnecessary blank lines after import statements (e.g., after `from mindsdb.api.http.start import start`) reduce code compactness and readability.

Scores:

  • Production Impact: 1
  • Fix Specificity: 2
  • Urgency Impact: 1
  • Total Score: 4

Reason for filtering: The comment addresses unnecessary blank lines after import statements, which has minimal to no impact on production, is not urgent, and does not provide a critical or specific fix. The total score does not meet the required threshold.

Analysis: This is a low-impact, style-related suggestion with no production consequences and low urgency. It does not meet the inclusion threshold.

Copy link
Contributor

@torrmal torrmal left a comment

Choose a reason for hiding this comment

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

looks good to me

@hamishfagg hamishfagg merged commit 3262c5c into main May 30, 2025
22 checks passed
@hamishfagg hamishfagg deleted the fix/a2a_config_get branch May 30, 2025 04:49
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants