feat: Add local MCP server for result fetching and readout analysis #400
Conversation
neelay-aign
requested a review
from helmut-hoffer-von-ankershoffen
as a code owner
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| pass | ||
|
|
||
| # Not a valid run_id, try to find a run by external_id | ||
| runs = list(client.runs.list(external_id=identifier, page_size=1)) |
There was a problem hiding this comment.
Bug: The token command always exits with an error code (1) due to a misplaced sys.exit(1) call, even on successful execution.
Severity: HIGH
Suggested Fix
Move the sys.exit(1) call to be inside the except Exception as e: block. This ensures that the command only exits with an error code when an exception is actually caught, and exits with the default success code (0) otherwise.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/aignostics/mcp/_server.py#L120
Potential issue: In the `token` command function within
`src/aignostics/platform/_cli.py`, the `sys.exit(1)` call is incorrectly placed at the
end of the function, outside of the `except` block. As a result, the command will always
exit with a status code of 1, indicating an error, even when the token is successfully
retrieved and printed. This behavior breaks standard command-line tool conventions,
where a zero exit code signifies success, and will cause issues for any scripts or
automation that rely on the exit code to determine the command's outcome.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is
|
|
| run_server() | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Do we ever expect to execute this script directly?
|
|
||
| # Download | ||
| if artifact.download_url: | ||
| response = requests.get(artifact.download_url, timeout=300) |
There was a problem hiding this comment.
This could reuse the download_file function to get integrity checking out of the box.
| return Run(self._api, run_id) | ||
|
|
||
| @classmethod | ||
| def _from_token(cls, token: str) -> "Client": |
There was a problem hiding this comment.
Can probably be gotten rid of now. There were some auth issues in early development which were solved using this explicit method. Will investigate.
| class Environment(StrEnum): | ||
| """Platform environment options.""" | ||
|
|
||
| PRODUCTION = "production" | ||
| STAGING = "staging" | ||
|
|
||
|
|
||
| # Environment API roots | ||
| ENV_API_ROOTS = { | ||
| Environment.PRODUCTION: "https://platform.aignostics.com", | ||
| Environment.STAGING: "https://platform-staging.aignostics.com", | ||
| } |
There was a problem hiding this comment.
The API root is already defined in the settings. I'd suggest pulling it from there.
There was a problem hiding this comment.
(Agree in principle with controlling the URLs with a single environment setting but then we should propagate that change to the entire SDK).
| def get_readouts_dir() -> Path: | ||
| """Get the readouts directory from environment or use default. | ||
| The directory can be configured via AIGNOSTICS_MCP_READOUTS_DIR. | ||
| Default is ~/aignostics_readouts (visible in home directory). | ||
| Returns: | ||
| Path to the readouts directory. | ||
| """ | ||
| env_dir = os.environ.get("AIGNOSTICS_MCP_READOUTS_DIR") | ||
| if env_dir: | ||
| return Path(env_dir) | ||
| return DEFAULT_CACHE_DIR |
There was a problem hiding this comment.
Suggest using the cache_dir defined in the settings.
Additionally, if I understand the current implementation correctly, users could easily end up downloading readouts twice:
- Once when they download run results using the GUI or CLI (
application run result download ...) -> readouts are downloaded to<cache_dir>/results/<run_id>/<external_item_id>/... - Once when they work with the readouts in the MCP server -> readouts are downloaded to a different location
Ideally, we would use the same location: if the user already downloaded all results for a run, the MCP server should simply open those without downloading them again.
No description provided.