fix(mcp): fix argo_list_experiments ignoring rollout_name parameter #83
Conversation
The rollout_name parameter was accepted but not used in the label selector.
The original code had an incomplete label selector 'rollouts-pod-template-hash'
that was missing the actual rollout name value.
This fix implements filtering by experiment name prefix, leveraging Argo
Rollouts' naming convention where experiment names follow the pattern:
{rollout-name}-{podHash}-{revision}-{stepIndex}.
Changes:
- Add json import for parsing kubectl output
- Fetch experiments in JSON format when filtering by rollout name
- Filter experiments by checking if name starts with rollout name
- Format filtered results as readable table (NAME, STATUS, AGE)
- Return clear message when no experiments found for the rollout
- Update parameter description and docstring
Fixes skyflo-ai#79
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReworked Changes
Sequence Diagram(s)(omitted — change is internal filtering and does not introduce multi-component sequential interactions warranting a diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)mcp/tools/argo.py (1)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mcp/tools/argo.py (1)
291-293: Consider logging or returning a clearer error on JSON parse failure.Silently passing on
JSONDecodeErrorand returning the raw JSON output could confuse users. Consider either logging a warning or returning a more explicit error message.🔎 Suggested improvement
except json.JSONDecodeError: - # If JSON parsing fails, return original output - pass + # If JSON parsing fails, return an error message + result["output"] = f"Failed to parse experiment data. Raw output: {result['output'][:200]}..." + result["error"] = TrueAlternatively, if you prefer silent degradation, at minimum add logging for debugging:
+import logging +logger = logging.getLogger(__name__) + except json.JSONDecodeError as e: - # If JSON parsing fails, return original output - pass + logger.warning(f"Failed to parse experiments JSON: {e}") + # Fall through to return original output
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/tools/argo.py
🧰 Additional context used
🧬 Code graph analysis (1)
mcp/tools/argo.py (1)
mcp/utils/commands.py (1)
run_command(8-50)
🔇 Additional comments (1)
mcp/tools/argo.py (1)
256-268: Filtering approach looks correct.The implementation correctly addresses the issue by:
- Using JSON output when filtering is needed
- Applying client-side prefix matching based on Argo's naming convention
- Falling back to wide output when no filter is specified
This is a sensible workaround given Argo Rollouts experiments don't have a reliable label for parent rollout.
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
Remove all redundant comments as well.
mcp/tools/argo.py
Outdated
| result = await run_command("kubectl", cmd_parts) | ||
|
|
||
| # If filtering by rollout name, parse JSON and filter experiments | ||
| if rollout_name and result.get("output") and not result.get("error"): |
There was a problem hiding this comment.
The function is declared to return ToolOutput, which is a TypedDict with keys output: str and error: bool. However, when rollout_name is provided, the code uses result.get("output") and result.get("error"), then mutates result["output"] directly.
This maintains consistency with the TypedDict contract:
| if rollout_name and result.get("output") and not result.get("error"): | |
| if rollout_name and result["output"] and not result["error"]: |
mcp/tools/argo.py
Outdated
| output_lines = ["NAME\t\tSTATUS\t\tAGE"] | ||
| for exp in filtered: | ||
| name = exp.get("metadata", {}).get("name", "") | ||
| status = exp.get("status", {}).get("phase", "Unknown") | ||
| created = exp.get("metadata", {}).get("creationTimestamp", "") | ||
| output_lines.append(f"{name}\t{status}\t{created}") |
There was a problem hiding this comment.
The column header says "AGE" but displays creationTimestamp (e.g., 2026-01-05T10:30:00Z). This is inconsistent with kubectl output, which shows relative age (e.g., 5m, 2h, 3d).
CodeRabbit raised this. It must be fixed.
Add a helper function to convert ISO timestamps to relative age:
from datetime import datetime, timezone
def _format_age(timestamp: str) -> str:
"""Convert ISO timestamp to kubectl-style relative age."""
try:
created = datetime.fromisoformat(timestamp.replace("Z", "+00:00"))
delta = datetime.now(timezone.utc) - created
seconds = int(delta.total_seconds())
if seconds < 0:
return "0s"
if seconds < 60:
return f"{seconds}s"
if seconds < 3600:
return f"{seconds // 60}m"
if seconds < 86400:
return f"{seconds // 3600}h"
return f"{seconds // 86400}d"
except (ValueError, TypeError):
return timestampThen use _format_age(created) instead of created directly.
mcp/tools/argo.py
Outdated
| except json.JSONDecodeError: | ||
| # If JSON parsing fails, return original output | ||
| pass |
There was a problem hiding this comment.
Return an error indicating the parse failure:
| except json.JSONDecodeError: | |
| # If JSON parsing fails, return original output | |
| pass | |
| except json.JSONDecodeError as e: | |
| result["output"] = f"Failed to parse experiment data: {e}" | |
| result["error"] = True |
mcp/tools/argo.py
Outdated
| output_lines = ["NAME\t\tSTATUS\t\tAGE"] | ||
| for exp in filtered: | ||
| name = exp.get("metadata", {}).get("name", "") | ||
| status = exp.get("status", {}).get("phase", "Unknown") | ||
| created = exp.get("metadata", {}).get("creationTimestamp", "") | ||
| output_lines.append(f"{name}\t{status}\t{created}") |
There was a problem hiding this comment.
The header uses double tabs (\t\t) but the data rows use single tabs (\t). This causes misaligned columns. Use consistent tab counts or, better, use string formatting with fixed widths.
- Add _format_age helper to convert ISO timestamps to relative age (5m, 2h, 3d) - Use direct TypedDict access (result[key]) instead of .get() for consistency - Improve error handling: return error message on JSONDecodeError - Fix column alignment using f-string width formatting - Remove redundant comments
|
@KaranJagtiani i have made the changes please have a look |
| rollout_name: Optional[str] = Field( | ||
| default=None, | ||
| description="The name of the rollout to get experiments for (if not specified, gets all experiments)", | ||
| description="The name of the rollout to filter experiments by. Experiments are filtered by checking if their name starts with the rollout name (Argo Rollouts naming convention: {rollout-name}-{hash}-{revision}-{step}). If not specified, returns all experiments.", |
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
- When all_namespaces is true you still return a table without namespace. That drops required context and is a regression vs kubectl get -A. Include NAMESPACE in the header and rows when all_namespaces is set, or always include it.
- Add test coverage
| """Get Argo Rollouts experiments, optionally filtered by rollout name. | ||
| When rollout_name is provided, experiments are filtered by checking if their | ||
| name starts with the rollout name, following Argo Rollouts' naming convention | ||
| where experiment names are: {rollout-name}-{podHash}-{revision}-{stepIndex}. | ||
| """ |
2 participants
Summary
Fixes #79
The
argo_list_experimentstool was accepting arollout_nameparameter but not effectively using it to filter experiments due to an incorrect label selector.Problem
The original implementation attempted to use a label selector that did not exist or was malformed:
The label selector
rollouts-pod-template-hashwas incomplete—it didn't include the actual rollout name or a valid selector format (key=value).Solution
After researching Argo Rollouts internals, I found that:
rollouts.argoproj.io/rolloutlabel.However, Argo Rollouts uses a reliable naming convention where experiment names follow the pattern:
{rollout-name}-{podHash}-{revision}-{stepIndex}The fix implements filtering by experiment name prefix using JSON output parsing:
Changes
mcp/tools/argo.py:Added
import jsonfor parsing kubectl output.Modified
argo_list_experiments()to:Fetch experiments in JSON format when filtering by rollout name.
Filter experiments by checking if their name starts with the rollout name.
Format filtered results as a readable table (NAME, STATUS, AGE).
Return a clear message when no experiments are found for the rollout.
Updated parameter description to explain the filtering mechanism.
Added detailed docstring explaining the filtering behavior.
Resources
rollout/experiment.gohttps://argo-rollouts.readthedocs.io/en/stable/features/experiment/- Experiment naming convention:fmt.Sprintf("%s-%s-%s-%d", r.Name, podHash, revision, currentStep)