Triage CLI - #717
Conversation
PR Summary by QodoAdd
AI Description
Diagram
High-Level Assessment
Files changed (23)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
7 rules 1.
|
![qodo-for-packit[bot]](https://avatars.githubusercontent.com/in/4066991?s=60&v=4){kind=small}
| result_dir = Path(os.environ.get("GIT_REPO_BASEPATH", "/git-repos")) / jira_issue | ||
| result_dir.mkdir(parents=True, exist_ok=True) | ||
| (result_dir / "triage_result.json").write_text(output.model_dump_json(indent=2), encoding="utf-8") |
There was a problem hiding this comment.
3. Jira key path traversal 🐞 Bug ⛨ Security
Direct-mode triage writes triage_result.json to Path(GIT_REPO_BASEPATH) / jira_issue and the CLI reads from Path(GIT_REPOS_HOST) / issue, but neither validates that the Jira key is not absolute or contains .., allowing reads/writes outside the intended work directory. This can corrupt or disclose local files when the issue string comes from untrusted automation or accidental misuse.
Agent Prompt
### Issue description
`jira_issue` / `issue` is used as a filesystem path segment in both the CLI and direct-mode agent. Without validation, values like `../../tmp/x` or `/etc` can escape the intended base directory.
### Issue Context
Other code paths already validate Jira keys before using them in filesystem operations (rejecting absolute paths and `..`). The new direct-mode result persistence and CLI result reading should follow the same pattern.
### Fix Focus Areas
- ymir/agents/triage_agent.py[865-929]
- ymir/cli/main.py[120-159]
- ymir/agents/tasks.py[111-116]
### Recommended fix
- Add a shared validation helper (or inline checks) that rejects:
- empty strings
- absolute paths (`Path(x).is_absolute()`)
- any segment containing `..`
- (optionally) enforce a Jira-key regex like `^[A-Z][A-Z0-9]+-\d+$`
- Apply it:
- in `ymir.cli.main.triage()` before constructing `issue_upper` paths
- in `ymir.agents.triage_agent.main()` direct-mode branch before creating `result_dir`
- Use the validated/normalized Jira key (e.g., uppercased) consistently for directory naming.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Technically true, but to exploit this, you would need an access to the host, and have permissions to access the file. And why would you then try to exfiltrate it in this convoluted way.
| | `ymir_todo` | Maintainer-facing trigger for an e2e run | Fetcher swaps it for `ymir_triage_in_progress` on enqueue; only honored when the changelog shows the label was added by a member of the `Red Hat Employee` Jira group (verified per-issue, not via JQL). The triage run posts an ack comment and a result comment so the requester gets feedback. Default is silent — without `ymir_todo`, no comments are posted. | | ||
| | `ymir_consolidate_base` | Mark a backport MR for consolidation (base) | Paired with `ymir_consolidate_next` on another issue for the same package/branch. The fetcher matches the pair, submits a targeted consolidation job, removes both labels, and posts comments. | | ||
| | `ymir_consolidate_next` | Mark a backport MR for consolidation (next) | Must be on a different issue than `ymir_consolidate_base`, for the same package/branch. | | ||
| | `ymir_manual_trigger` | Dedup anchor for CLI-triggered runs | Set by the CLI at the start of a user-triggered triage run, before the workflow executes. Prevents the fetcher from picking up the same issue in parallel. Persists after completion alongside the terminal resolution label. | |
There was a problem hiding this comment.
I really like this that we'd be able to track how many people will use the CLI
| _ENV_KEYS = ("RHEL_CONFIG_PATH", "MOCK_JIRA", "JIRA_MOCK_FILES_HOST", "GIT_REPOS_HOST") | ||
|
|
||
|
|
||
| def check_credentials() -> None: |
There was a problem hiding this comment.
this function brings a ton of value and should help the adoption a lot
nforro
left a comment
nforro
left a comment
There was a problem hiding this comment.
LGTM, but Claude has some remarks (nothing major):
1. Compose changes to mcp-gateway affect all profiles, not just CLI
The diff parameterizes GIT_REPO_BASEPATH, GIT_CONFIG_GLOBAL, and the git-repos volume mount in the *mcp-gateway anchor:
# Before (hardcoded)
- GIT_REPO_BASEPATH=/git-repos
- git-repos:/git-repos
# After (env-var-driven)
- GIT_REPO_BASEPATH=${GIT_REPO_BASEPATH:-/git-repos}
- ${GIT_REPOS_HOST:-git-repos}:${GIT_REPO_BASEPATH:-/git-repos}:zDefaults preserve existing behavior, but these apply to agents, supervisor, and e2e-test profiles too (via the anchor). If someone accidentally exports GIT_REPOS_HOST or GIT_REPO_BASEPATH in a prod-like environment, the gateway's volume mount silently changes. The :z SELinux label addition is actually a consistency fix (other volumes already had it), but it's still a behavioral change for non-CLI profiles.
Consider documenting as intentional scope-widening, or limiting the override to mcp-gateway-cli only.
2. Resolution.ERROR leaves the issue stuck with only ymir_manual_trigger
The label lifecycle:
| Outcome | ymir_manual_trigger |
Resolution label |
|---|---|---|
| Success | stays | added |
| Crash (exception) | removed | — |
Resolution.ERROR |
stays | skipped |
On Resolution.ERROR, the issue keeps ymir_manual_trigger (blocking the fetcher) but gets no resolution label. The issue is effectively claimed with no indication of what happened — diagnosis requires checking container logs. The crash path removes ymir_manual_trigger (allowing fetcher retry), suggesting transient failures should be retryable — Resolution.ERROR is arguably closer to that category.
Consider writing TRIAGE_ERRORED even in direct mode, or documenting this stuck state in the CLI help.
3. stale_labels list is a snapshot from before the workflow runs
current_labels, _ = await tasks.get_jira_issue_metadata(jira_issue)
# ...workflow runs (can take minutes)...
stale_labels = [
label for label in current_labels
if label.startswith("ymir_") and label != JiraLabels.MANUAL_TRIGGER.value
]If another process adds or removes ymir_* labels while the workflow runs, stale_labels would miss them or try to remove labels that are already gone. Low risk in practice (CLI issues shouldn't be fetcher-processed concurrently), but worth documenting the assumption.
4. _normalize_protobuf_ids doesn't handle link IDs
The function only normalizes span-level traceId, spanId, parentSpanId. Span links also carry these fields in base64 after MessageToDict. The trace server doesn't currently process link data, so no runtime impact — but if it ever does, base64 IDs containing / would break URL routing.
5. f-string in logger.warning — style inconsistency
trace_server/server.py line 429:
logger.warning(f"POST /v1/traces rejected: invalid protobuf: {e}")Other logging calls in the same file use %s-style lazy formatting. The f-string eagerly evaluates str(e) even when WARNING is disabled.
6. functools.cache on _compose_base_cmd never cleared in tests
The cache persists for the entire test process. Each test gets a unique tmp_path so there are no cache hits across tests, but within a single test that calls both start_services and stop_services on the same path (e.g. TestTriageContainerLifecycle), the second call hits the cache and skips the mocked detect_compose_cmd(). Works by accident since the mock returns the same value — but the cache masks the second call.
7. load_dotenv side effects leak between tests
triage() calls load_dotenv(override=False) which sets env vars like CHAT_MODEL. These aren't in _ENV_KEYS and aren't restored in the finally block. Tests that don't use patch.dict(os.environ, clear=True) (e.g. TestEnvVarRestoration) leak these values to subsequent tests. Not currently causing failures, but a latent ordering dependency.
| if label.startswith("ymir_") and label != JiraLabels.MANUAL_TRIGGER.value | ||
| ] | ||
| try: | ||
| await tasks.set_jira_labels( |
There was a problem hiding this comment.
I think we should try to minimise labelling of Jiras during the CLI triggered runs. The labels are mainly for coordinating what's happening in the service (dedup, routing, status tracking), they don't add much value when triggered locally where the user sees what's happening in their terminal. A lot of service logic relies on these and this could cause some unexpected behaviours (e.g. if the CLI run fails, the label set before the workflow stays and blocks the fetcher from picking up the issue in the service). Maybe we could stick to adding the label to be able to see issues that were invoked from CLI after the run finishes successfully?
There was a problem hiding this comment.
Blocking the fetcher is actually what I was going for. Running Ymir through CLI is a valid, but a fundamentally irregular operation. I'd rather avoid possible race between fetcher and CLI. If user wants to triage something outside of the regular workflow, we should trust them that they don't want the regular workflow to touch it, whatever reasons they may have.
That being said. I do understand why do you want the label removed on a failed run.
There was a problem hiding this comment.
Thanks for clarification, blocking the fetcher with the label makes sense, agreed.
My concern was also about error comments to be posted to Jira (because of the user_triggered=True). For CLI, the user sees errors in their terminal, so posting them to Jira too feels unnecessary. This behavior was designed for ymir_todo where the user only has Jira as feedback channel.
Most of these seem valid. And implementing adjustments should be very easy. |
|
Looks like something is wrong with the gateway. |
Should be fixed by f96cd96. |
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
just one note, otherwise LGTM, thank you
| no_auto_chain: bool = typer.Option( | ||
| False, "--no-auto-chain", help="Do not push results to downstream agent queues" | ||
| ), |
There was a problem hiding this comment.
I think this whole functionality can be removed here, as we don't have the valkey queue and propagation to downstream agents/queues
Signed-off-by: Jiri Podivin <jpodivin@redhat.com> Assisted-by: Claude Opus 4.6
4 participants
Triage can now be executed using CLI command
ymir. The command verifies credentials, spins up containers, and starts the workflow. Once it's done, the containers are stopped. The sole exception is the trace-server, so that developers can access the collected trace information and see how the agent behaved. The dotenv package was introduced to handle env vars, as most of the credentials are managed this way, and they have to be verified and synced across containers and host.When the workflow is executed, the given JIRA is marked with
ymir_manual_triggerlabel. This blocks the the JIRA from being picked up by the fetcher. The intent here is to clearly mark this workflow as somewhat irregular. The CLI is not intended to be a primary way for anyone to use Ymir. This way we can also see how often people invoke Ymir manually, by running as simple query in Jira.This PR is also a base for possible expansion of CLI to other parts of the workflow.
Trace server
I didn't want to run more containers than necessary, so to skip
otel-collector, I've implemented protobuf parsing in the trace-server. This does require two new dependencies,opentelemetry-protoandprotobuf. The image size is comfortable 265 MB.But I understand that this is a matter of opinion.
Changes to ymir/common/pyproject.toml
This code was developed in parallel to ymir/common/pyproject.toml so I had put the
merge_queue.pyon the explicit list to get the package working. Depending on the merge order, this part can be dropped.RHEL_CONFIG_PATH env var
To make sure that all workflows load the config pointed to by
--secretstheRHEL_CONFIG_PATHis set by the CLI, and later used by theload_rhel_configfunction.RELEASE NOTES BEGIN
Triage workflow can now be triggered using CLI.
RELEASE NOTES END