Register missing unprivileged tools in MCP gateway

[nforro]

No description provided.

[qodo-for-packit]

PR Summary by Qodo

Register all unprivileged tools in MCP gateway and enforce via pre-push hook
✨ Enhancement ⚙️ Configuration changes 🕐 10-20 Minutes

Description

• Register previously unregistered unprivileged Tool implementations in the MCP gateway.
• Add a pre-push hook to detect future unregistered tools via AST-based scanning.
• Allow bypassing the check via SKIP_GATEWAY_CHECK for exceptional cases.

Diagram

graph TD
  A(["git push"]) --> B(["pre-commit: pre-push"]) --> C["scripts/check-unprivileged-gateway.py"] --> D[["ymir/tools/unprivileged/"]] --> E["unprivileged/gateway.py"] --> F(["MCPServer.register_many"])

  subgraph Legend
    direction LR
    _u(["User action"]) ~~~ _hook(["Hook"]) ~~~ _script["Script"] ~~~ _dir[["Directory"]] ~~~ _svc(["Service/API"])
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Runtime auto-discovery/registration

• ➕ Eliminates the need to manually update gateway imports/registration
• ➕ Ensures new tools are available immediately when added
• ➖ Higher operational risk: implicit exports may accidentally expose tools
• ➖ More complexity (pkgutil/entrypoints), harder to audit tool surface area

2. CI-only enforcement (no pre-push hook)

• ➕ Avoids adding friction to local developer pushes
• ➕ Guarantees consistent enforcement in a single place
• ➖ Slower feedback loop; missing registrations discovered after pushing
• ➖ Does not prevent repeated back-and-forth in PRs

3. Unit test asserting gateway coverage

• ➕ Runs in standard test pipelines; easier to version/control than local hooks
• ➕ Can be more precise by inspecting register_many() arguments directly
• ➖ Still post-change feedback unless combined with local hooks
• ➖ May require heavier test harness setup depending on imports/config

Recommendation: The PR’s approach (explicit gateway registration + fast pre-push guard) is a good balance: it preserves an auditable, explicit tool surface area while preventing regressions early. If enforcement needs to be stronger across all contributors, consider adding the same script to CI as well (keeping the pre-push hook as a convenience).

Files changed (3) +106 / -0

Enhancement (1) +13 / -0

gateway.pyImport and register missing unprivileged tools in MCP gateway +13/-0

Import and register missing unprivileged tools in MCP gateway

• Adds missing tool imports and includes them in the MCPServer.register_many() list so they are exposed through the unprivileged MCP gateway. Also registers GitPreparePackageSources alongside other wicked_git tools.

ymir/tools/unprivileged/gateway.py

Other (2) +93 / -0

.pre-commit-config.yamlAdd pre-push hook to validate unprivileged gateway tool registration +7/-0

Add pre-push hook to validate unprivileged gateway tool registration

• Registers a new local pre-push hook that runs a Python script to ensure all unprivileged tools are present in the MCP gateway. The hook is configured to always run and not depend on filenames.

.pre-commit-config.yaml

check-unprivileged-gateway.pyIntroduce AST-based checker for unprivileged Tool registration coverage +86/-0

Introduce AST-based checker for unprivileged Tool registration coverage

• Adds a new script that scans ymir/tools/unprivileged/ for classes subclassing Tool and verifies they are instantiated in the gateway module. Supports opt-out via SKIP_GATEWAY_CHECK and prints a focused missing-tools report.

scripts/check-unprivileged-gateway.py

[gemini-code-assist]

Code Review

This pull request adds a pre-push hook script, check-unprivileged-gateway.py, and registers it in .pre-commit-config.yaml to ensure all unprivileged MCP tools are properly registered in the gateway. It also registers several missing tools in ymir/tools/unprivileged/gateway.py. The review feedback suggests improving the robustness of the script by specifying UTF-8 encoding when reading files, ignoring base classes, handling attribute-based base classes, and checking for both Tool and CloneableTool base classes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[TomasTomecek]

LGTM, very nice idea

[qodo-for-packit]

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 7 rules

1. No tests for tool_options 📘 Rule violation ▣ Testability

Description

ymir/tools/privileged/gateway.py now constructs a shared tool_options dict and passes it into
every privileged tool instance, but there are no unit tests verifying this updated gateway behavior.
This violates the requirement to add/update unit tests for changes under ymir/tools/privileged/,
increasing risk of regressions in privileged tool wiring.

Code

ymir/tools/privileged/gateway.py[R120-131]

+    tool_options: dict = {}
    mcp = MCPServer(config=config)
    mcp.register_many(
        [
-            BuildPackageTool(),
-            DownloadArtifactsTool(),
-            CreateZstreamBranchTool(),
-            AddBlockingMergeRequestCommentTool(),
-            AddMergeRequestCommentTool(),
-            AddMergeRequestLabelsTool(),
-            CloneRepositoryTool(),
-            ForkRepositoryTool(),
-            GetAuthorizedCommentsFromMergeRequestTool(),
-            GetFailedPipelineJobsFromMergeRequestTool(),
-            GetInternalRhelBranchesTool(),
-            GetMergeRequestDetailsTool(),
-            GetPatchFromUrlTool(),
-            OpenMergeRequestTool(),
-            PushToRemoteRepositoryTool(),
-            RetryPipelineJobTool(),
-            FetchGitlabMrNotesTool(),
-            SearchGitlabProjectMrsTool(),
-            GetErratumTool(),
-            GetErratumBuildNvrTool(),
-            GetTestingFarmRequestTool(),
-            ReproduceTestingFarmRequestTool(),
-            AddJiraAttachmentsTool(),
-            AddJiraCommentTool(),
-            ChangeJiraStatusTool(),
-            CheckCveTriageEligibilityTool(),
-            EditJiraLabelsTool(),
-            GetJiraAttachmentTool(),
-            GetJiraDetailsTool(),
-            GetJiraDevStatusTool(),
-            GetJiraPullRequestsTool(),
-            SearchJiraIssuesTool(),
-            SetJiraFieldsTool(),
-            SetPreliminaryTestingTool(),
-            UpdateJiraCommentTool(),
-            VerifyIssueAuthorTool(),
-            DownloadSourcesTool(),
-            PrepSourcesTool(),
-            UploadSourcesTool(),
-            ZStreamSearchTool(),
-            AnalyzeLogsTool(),
-            MaintainerRulesTool(),
+            BuildPackageTool(options=tool_options),
+            DownloadArtifactsTool(options=tool_options),
+            CreateZstreamBranchTool(options=tool_options),
+            AddBlockingMergeRequestCommentTool(options=tool_options),
+            AddMergeRequestCommentTool(options=tool_options),
+            AddMergeRequestLabelsTool(options=tool_options),
+            CloneRepositoryTool(options=tool_options),
+            ForkRepositoryTool(options=tool_options),

Evidence

The checklist requires new/updated unit tests for modifications under ymir/tools/privileged/. The
new gateway code introduces shared tool_options and passes it into each tool constructor, but the
existing privileged gateway tests only cover credential redaction and do not assert anything about
tool registration or options passing.

Rule 1589: Require unit tests for changes to privileged tools (ymir/tools/privileged/, esp. distgit.py)
ymir/tools/privileged/gateway.py[120-165]
ymir/tools/privileged/tests/unit/test_gateway.py[1-7]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ymir/tools/privileged/gateway.py` was changed to pass a shared `options` dict (`tool_options`) into each registered privileged tool, but the privileged tool test suite does not cover this behavior.

## Issue Context
Compliance requires that changes under `ymir/tools/privileged/` include new or updated unit tests that cover the changed behavior. A good test should fail if `tool_options` is removed or if tools stop receiving the shared dict.

## Fix Focus Areas
- ymir/tools/privileged/gateway.py[120-165]
- ymir/tools/privileged/tests/unit/test_gateway.py[1-200]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Gateway tool state isolated ✓ Resolved 🐞 Bug ≡ Correctness

Description

GitPreparePackageSources persists base_head_commit into self.options, and
GitPatchCreationTool later reads self.options["base_head_commit"], but the unprivileged gateway
constructs these as separate tool instances without a shared options dict. As a result, invoking
git_prepare_package_sources followed by git_patch_create via the MCP gateway will fail because
the base commit is never visible to the patch-creation tool instance.

Code

ymir/tools/unprivileged/gateway.py[R90-95]

            GitPatchApplyFinishTool(),
            GitPatchCreationTool(),
            GitLogSearchTool(),
+            GitPreparePackageSources(),
            RunPackagePrepTool(),
            BuildSrpmTool(),

Evidence

The unprivileged gateway registers GitPreparePackageSources() without providing a shared options
object. The tool implementation writes base_head_commit into self.options, and
GitPatchCreationTool later reads self.options.get("base_head_commit") and errors if missing;
local workflow construction passes the same local_tool_options dict into both tools, demonstrating
they are intended to share a single options/state dict.

ymir/tools/unprivileged/gateway.py[63-103]
ymir/tools/unprivileged/wicked_git.py[16-84]
ymir/tools/unprivileged/wicked_git.py[481-529]
ymir/agents/backport_agent.py[128-156]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ymir/tools/unprivileged/gateway.py` registers git workflow tools as independent instances with no shared `options` state. Several tools in `ymir/tools/unprivileged/wicked_git.py` communicate by writing/reading `self.options` (e.g., `base_head_commit`), so the workflow breaks when the tools are used via the MCP gateway.

## Issue Context
- `GitPreparePackageSources` writes `self.options["base_head_commit"]`.
- `GitPatchCreationTool` expects to read the same key from `self.options` and raises `ToolError` if it is missing.
- Local in-process workflows explicitly pass the same `local_tool_options` dict into both tools, showing the intended shared-state mechanism.

## Fix Focus Areas
- ymir/tools/unprivileged/gateway.py[53-103]
- ymir/tools/unprivileged/wicked_git.py[16-89]
- ymir/tools/unprivileged/wicked_git.py[481-529]
- ymir/agents/backport_agent.py[128-156]

## Suggested fix
In `ymir/tools/unprivileged/gateway.py`, create a single shared options dict (e.g., `tool_options = {"working_directory": None}`) and pass it into all tools that are expected to share workflow state (at minimum `GitPreparePackageSources`, `GitPatchCreationTool`, and any other git workflow tools that read/write `self.options`). This ensures `base_head_commit` (and similar cross-step state) is visible across the workflow when invoked over MCP.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Undeclared nitrate dependency 🐞 Bug ☼ Reliability

Description

ymir.tools.unprivileged.gateway now imports AnalyzeEwaTestRunTool, whose module imports
nitrate, but ymir-tools dependencies (from ymir/tools/requirements.txt) do not include
nitrate. As a result, installs that use ymir-tools (and its ymir-unprivileged-gateway
entrypoint) without the root package deps can fail at import-time with ImportError before serving.

Code

ymir/tools/unprivileged/gateway.py[11]

+from ymir.tools.unprivileged.analyze_ewa_testrun import AnalyzeEwaTestRunTool

Evidence

The gateway now imports AnalyzeEwaTestRunTool, which imports nitrate. ymir-tools dependencies
are sourced from ymir/tools/requirements.txt via ymir/tools/pyproject.toml, and that
requirements file does not include nitrate, so the gateway import can fail in ymir-tools-only
environments.

ymir/tools/unprivileged/gateway.py[9-18]
ymir/tools/unprivileged/analyze_ewa_testrun.py[1-10]
ymir/tools/pyproject.toml[5-18]
ymir/tools/requirements.txt[1-11]
requirements.txt[1-22]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ymir-unprivileged-gateway` now imports `AnalyzeEwaTestRunTool`, which imports the third-party `nitrate` module. However, `ymir-tools` package dependencies (derived from `ymir/tools/requirements.txt`) don’t list `nitrate`, so `ymir-tools` installs can fail at startup with `ImportError`.

### Issue Context
- `ymir/tools/unprivileged/analyze_ewa_testrun.py` imports `nitrate`.
- `ymir/tools/unprivileged/gateway.py` imports/registers `AnalyzeEwaTestRunTool`.
- `ymir/tools/pyproject.toml` declares dependencies dynamically from `ymir/tools/requirements.txt`, which currently does not include `nitrate`.

### Fix Focus Areas
- ymir/tools/requirements.txt[1-12]
- ymir/tools/unprivileged/analyze_ewa_testrun.py[1-10]
- ymir/tools/unprivileged/gateway.py[9-18]

### Suggested fix
Add `nitrate>=...` to `ymir/tools/requirements.txt` (preferred if the tool is always shipped/registered), or make `AnalyzeEwaTestRunTool` registration conditional (lazy import / optional tool) if `nitrate` is intended to remain optional.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Stale shared tool state 🐞 Bug ☼ Reliability

Description

ymir/tools/unprivileged/gateway.py now shares one mutable tool_options dict across all
registered tools, so keys written by one workflow can persist and affect later, unrelated tool runs
in the same gateway process. For example, GitPreparePackageSources stores base_head_commit in
self.options, and GitPatchCreationTool later consumes it without scoping it to the requested
repository_path, which can generate a patch from the wrong base (or fail) if the gateway is used
for a different repo/workflow afterward.

Code

ymir/tools/unprivileged/gateway.py[R65-75]

+    # Shared options dict so tools can communicate cross-step state
+    # (e.g. base_head_commit set by GitPreparePackageSources / ApplyDownstreamPatchesTool,
+    # read by GitPatchCreationTool).
+    tool_options: dict = {}
    mcp = MCPServer(config=config)
    mcp.register_many(
        [
-            RunShellCommandTool(),
-            DistgitDetectorTool(),
-            GetCWDTool(),
-            RemoveTool(),
-            GetPackageInfoTool(),
-            AddChangelogEntryTool(),
-            UpdateReleaseTool(),
-            CreateTool(),
-            ViewTool(),
-            InsertTool(),
-            InsertAfterSubstringTool(),
-            StrReplaceTool(),
-            SearchTextTool(),
-            UpstreamSearchTool(),
-            ExtractUpstreamRepositoryTool(),
-            CloneUpstreamRepositoryTool(),
-            FindBaseCommitTool(),
-            ApplyDownstreamPatchesTool(),
-            CherryPickCommitTool(),
-            CherryPickContinueTool(),
-            VersionMapperTool(),
-            GitPatchApplyTool(),
-            GitPatchApplyFinishTool(),
-            GitPatchCreationTool(),
-            GitLogSearchTool(),
-            RunPackagePrepTool(),
-            BuildSrpmTool(),
+            RunShellCommandTool(options=tool_options),
+            DistgitDetectorTool(options=tool_options),
+            GetCWDTool(options=tool_options),
+            RemoveTool(options=tool_options),

Evidence

The gateway now passes a single shared dict to every tool, and multiple tools read/write workflow
state into self.options without any scoping or reset, making later tool behavior depend on
previous runs in the same process.

ymir/tools/unprivileged/gateway.py[65-105]
ymir/tools/unprivileged/wicked_git.py[16-85]
ymir/tools/unprivileged/wicked_git.py[481-525]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The unprivileged gateway creates a single `tool_options` dict and passes it to every tool instance. Tools write workflow-specific state into `self.options` (e.g., `base_head_commit`), but that state is not scoped to a specific repository/client/workflow, so later runs can observe stale values and behave incorrectly.

### Issue Context
- `GitPreparePackageSources` writes `self.options["base_head_commit"]`.
- `GitPatchCreationTool` reads `self.options.get("base_head_commit")` and uses it for `git format-patch` against the caller-provided `repository_path`, without validating that the commit belongs to that repo or that the state is still relevant.
- The gateway is a long-lived process (`mcp.serve()`), so this state can leak across sequential workflows.

### Fix Focus Areas
- ymir/tools/unprivileged/gateway.py[65-105]

### Suggested fix approaches
Pick one:
1) **Scope state by repository**: store state under a key derived from `repository_path` (e.g. `tool_options["repos"][str(repo_path)]["base_head_commit"]`) and update the relevant tools to read/write within that namespace.
2) **Reset state explicitly**: clear workflow keys (`base_head_commit`, `base_tag_commit`, `fix_commit`, etc.) at the start of a workflow entry tool (e.g., `git_prepare_package_sources`) and/or after `git_patch_create` completes.
3) **Add validation guardrails** (partial mitigation): before using `base_head_commit` in `git_patch_create`, verify it exists in `repository_path` (e.g., `git cat-file -e <sha>^{commit}`) and fail with a clear error prompting the user to re-run the preparatory step in that repo.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Registered-tools scan too broad 🐞 Bug ≡ Correctness

Description

find_registered_classes() claims to extract tools instantiated in the gateway’s register_many()
call, but it actually collects every Name() call anywhere in gateway.py. This can incorrectly
treat a tool as “registered” if it’s instantiated outside the register_many([...]) list, weakening
the hook’s ability to catch missing registrations.

Code

scripts/check-unprivileged-gateway.py[R49-56]

+def find_registered_classes(gateway_file: Path) -> set[str]:
+    """Find all tool classes instantiated in the gateway's register_many() call."""
+    tree = ast.parse(gateway_file.read_text())
+    registered: set[str] = set()
+    for node in ast.walk(tree):
+        if isinstance(node, ast.Call) and isinstance(node.func, ast.Name):
+            registered.add(node.func.id)
+    return registered

Evidence

The checker adds every Name() call from the entire gateway AST, and the gateway contains non-tool
calls (e.g., setup_logging(), apply_zstream_override_from_env(), MCPServerConfig(...),
int(...), main()) that would be included despite not being part of register_many([...]).

scripts/check-unprivileged-gateway.py[49-56]
ymir/tools/unprivileged/gateway.py[53-66]
ymir/tools/unprivileged/gateway.py[105-109]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The pre-push checker intends to validate that every unprivileged Tool subclass is instantiated inside `mcp.register_many([...])`. Today it over-approximates by collecting *all* `ast.Call` nodes where `node.func` is an `ast.Name`, regardless of whether they are inside the `register_many` list.

### Issue Context
This can create false negatives: a tool could be instantiated somewhere else (or incidentally called) and still be counted as registered even if missing from `register_many([...])`.

### Fix Focus Areas
- scripts/check-unprivileged-gateway.py[49-56]
- ymir/tools/unprivileged/gateway.py[53-103]

### Suggested fix
In `find_registered_classes()`:
1. Find the `ast.Call` where `node.func` is an `ast.Attribute` with `attr == "register_many"`.
2. Extract the first argument; if it’s an `ast.List`, iterate elements.
3. For each element that is `ast.Call` with `func` as `ast.Name`, add that name.
4. Optionally handle simple list concatenations / starred expressions if used in the gateway.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Tool scan skips parse failures 🐞 Bug ☼ Reliability

Description

find_tool_classes() silently drops any tool file that triggers SyntaxError during AST parsing
and does not handle read/decode failures from Path.read_text(). This can cause the hook to pass
even though it failed to scan all tools (or crash mid-scan), undermining the intended guarantee.

Code

scripts/check-unprivileged-gateway.py[R28-31]

+        try:
+            tree = ast.parse(py_file.read_text())
+        except SyntaxError:
+            continue

Evidence

The scanner explicitly continues on SyntaxError and does not catch file read/decode errors
around read_text(), so failures in reading/parsing tool modules can be silently ignored or crash
the hook.

scripts/check-unprivileged-gateway.py[23-32]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The pre-push hook is meant to enforce that *all* unprivileged Tool subclasses are registered. Currently, scan failures are either silently ignored (`SyntaxError`) or can crash the script (`UnicodeDecodeError` / `OSError` from `read_text()`), making results unreliable.

### Issue Context
Silent skipping means developers can push changes where the checker didn’t actually validate all tool files.

### Fix Focus Areas
- scripts/check-unprivileged-gateway.py[20-46]

### Suggested fix
- Read files with explicit encoding and safe error handling, e.g. `read_text(encoding="utf-8", errors="replace")` or catch `(OSError, UnicodeDecodeError)`.
- On `SyntaxError`, print a clear error including the file path and fail the hook (return 1), unless explicitly bypassed via `SKIP_GATEWAY_CHECK`.
- Consider passing `filename=str(py_file)` to `ast.parse(..., filename=...)` to improve diagnostics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[nforro]

Qodo, are you going to do a re-review?

[qodo-for-packit]

are you going to do a re-review?

Yes — if you push updates, I can re-review the PR.

If you want, I can also focus the re-review on specific findings, like finding 1, finding 2, or finding 3.

[nforro]

Yes — if you push updates, I can re-review the PR.

Can you do that now?

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit b148c94

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit a47af90

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 5173c75