Skip to content

Fix triage agent cloning to wrong path - #670

Merged
opohorel merged 3 commits into
packit:mainfrom
opohorel:triage_clone
Jul 9, 2026
Merged

Fix triage agent cloning to wrong path#670
opohorel merged 3 commits into
packit:mainfrom
opohorel:triage_clone

Conversation

@opohorel

@opohorel opohorel commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

The triage agent's clone_repository calls occasionally clone repos into /tmp inside the MCP gateway container. Since /tmp is not shared between the gateway and agent containers (only /git-repos is a shared PVC), the agent can't see the cloned repo and fails with "file not found" errors, wasting LLM tokens on recovery attempts.

Two-layer fix — prompt guidance (soft) + runtime guard (hard):

  • Prompt: Updated triage/prompt.j2 to explicitly instruct the LLM to pass clone_path=/git-repos/<issue>/<package> when calling clone_repository, matching the convention already used by the backport agent.
  • Tool guard: Added a resolved-path validation in CloneRepositoryTool._run() that rejects any clone_path outside GIT_REPO_BASEPATH with a clear ToolError. Uses Path.resolve() on both sides to block .. traversal. Runs before clean_stale_repositories() to skip unnecessary I/O on invalid requests.
  • Field description: Updated clone_path schema description to mention the /git-repos constraint, giving the LLM an additional signal via the tool schema.

opohorel added 2 commits July 9, 2026 13:54
Reject clone_path values outside the shared GIT_REPO_BASEPATH volume
(/git-repos) with a clear ToolError, preventing agents from cloning
into container-local paths like /tmp that are invisible to other
containers.  The guard resolves both sides (Path.resolve) to block
".." traversal attacks, and runs before clean_stale_repositories to
skip unnecessary I/O on invalid paths.

Also updates the clone_path field description to mention the /git-repos
constraint.

Assisted-by: Claude (Cursor)
Instruct the LLM to pass clone_path=/git-repos/<issue>/<package>
when calling clone_repository, so cloned repos land on the shared
volume visible to both the agent and MCP gateway containers.
Previously the prompt omitted clone_path, letting the model pick
arbitrary locations like /tmp that only exist inside the gateway.

Assisted-by: Claude (Cursor)
@qodo-for-packit

Copy link
Copy Markdown

PR Summary by Qodo

Guard triage clone_repository to always use shared /git-repos path

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Instruct triage prompt to pass clone_path under /git-repos for repository clones.
• Reject clone_repository calls that target paths outside the shared volume.
• Add unit tests covering allowed, disallowed, and path-traversal clone_path inputs.
Diagram

graph TD
  P["Triage prompt (Jinja2)"] --> A["Triage agent (LLM)"] --> T["CloneRepositoryTool"] --> V["Shared /git-repos volume"]
  V --> G["MCP gateway container"]
  V --> C["Agent container"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Make clone_path optional and default to /git-repos//
  • ➕ Eliminates a common LLM failure mode by removing the need to supply clone_path.
  • ➕ Keeps flexibility while maintaining safe defaults.
  • ➖ Tool must infer issue/package context or accept extra inputs, complicating the API.
  • ➖ May silently choose an unexpected location if inference is wrong.
2. Remove clone_path from the tool API entirely
  • ➕ Strongest guarantee against cloning outside shared storage.
  • ➕ Simplifies the tool contract for LLM usage.
  • ➖ Reduces flexibility for non-triage callers/tests that may want explicit paths.
  • ➖ Requires a single canonical directory layout decision for all use cases.
3. Share /tmp between gateway and agent containers
  • ➕ No tool/API changes; fixes visibility issue at infrastructure layer.
  • ➖ Bigger operational change; increases risk of incidental coupling and storage hygiene issues.
  • ➖ Still allows arbitrary clone locations and path traversal concerns.

Recommendation: Current approach (prompt guidance + hard runtime guard) is the best tradeoff: it prevents wasted runs immediately (guard), improves model behavior (prompt/schema hints), and keeps the tool API stable. The only follow-up worth considering is a safe default clone_path to further reduce reliance on prompt compliance.

Files changed (4) +62 / -3

Bug fix (1) +10 / -1
gitlab.pyReject clone_path outside shared basepath in CloneRepositoryTool +10/-1

Reject clone_path outside shared basepath in CloneRepositoryTool

• Adds a resolved-path guard in CloneRepositoryTool._run() to require clone_path to be under GIT_REPO_BASEPATH (default /git-repos). Uses Path.resolve() and is_relative_to() to block traversal and fails fast before stale cleanup I/O.

ymir/tools/privileged/gitlab.py

Tests (2) +48 / -0
test_jinja2_templates.pyAssert triage prompt renders clone_path guidance +2/-0

Assert triage prompt renders clone_path guidance

• Extends the triage template rendering test to assert the prompt includes clone_path and the expected /git-repos/<issue>/ prefix when an internal fix flow is rendered.

ymir/agents/tests/unit/test_jinja2_templates.py

test_gitlab.pyAdd unit coverage for clone_path basepath validation +46/-0

Add unit coverage for clone_path basepath validation

• Adds async tests verifying clone_repository rejects absolute paths outside the basepath, rejects traversal attempts via '..', and still succeeds for valid paths inside the shared volume.

ymir/tools/privileged/tests/unit/test_gitlab.py

Other (1) +4 / -2
prompt.j2Specify clone_path under /git-repos for triage cloning +4/-2

Specify clone_path under /git-repos for triage cloning

• Updates the triage prompt to explicitly pass clone_path=/git-repos/<issue>/<package> when calling clone_repository. Ensures clones land on the shared volume and remain visible across containers, including the fallback clone case.

ymir/agents/prompts/triage/prompt.j2

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the triage prompt to specify a clone path under the shared volume and implements path validation in the CloneRepositoryTool to prevent path traversal. Unit tests are also added to verify this validation. The feedback recommends strengthening the path validation to reject cases where the target path is exactly equal to the base path, preventing cloning directly into the root of the shared volume, and adding a corresponding test case.

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.

Comment thread ymir/tools/privileged/gitlab.py Outdated
Comment thread ymir/tools/privileged/tests/unit/test_gitlab.py
Disallow clone_path == GIT_REPO_BASEPATH itself (e.g. /git-repos)
to prevent creating .git in the shared volume root.

Assisted-by: Claude (Cursor)
@qodo-for-packit

qodo-for-packit Bot commented Jul 9, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 7 rules

Grey Divider


Action required

1. Basepath root clone allowed ✓ Resolved 🐞 Bug ☼ Reliability
Description
CloneRepositoryTool._run only checks that clone_path resolves under GIT_REPO_BASEPATH, but it still
accepts clone_path == basepath (because is_relative_to(basepath) is true for the base itself), so a
caller can run git init/checkout directly in /git-repos. This can pollute the shared workspace root
and can trigger git “dubious ownership” failures because the container only marks /git-repos/* (not
/git-repos itself) as a safe.directory.
Code

ymir/tools/privileged/gitlab.py[R466-470]

+        basepath = Path(os.getenv("GIT_REPO_BASEPATH", "/git-repos")).resolve()
+        resolved = clone_path.resolve()
+        if not resolved.is_relative_to(basepath):
+            raise ToolError(f"clone_path must be under {basepath} (the shared volume). Got: {clone_path}")
+        clone_path = resolved
Relevance

⭐⭐⭐ High

Team repeatedly accepts filesystem/path hardening; safe.directory configured for /git-repos/* not
root (PR458); path validation accepted (PR571).

PR-#458
PR-#571
PR-#489

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The current validation only rejects paths not under the base, but it does not reject the base
directory itself; the tool later runs git operations in that directory. The repo’s container setup
explicitly configures git safe.directory for /git-repos/* rather than /git-repos, indicating the
root is not intended to be treated as a repo working tree, and the shared base also stores
global/per-issue gitconfig files.

ymir/tools/privileged/gitlab.py[462-496]
Containerfile.mcp[69-74]
ymir/common/mock_repos.py[86-103]

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

## Issue description
`CloneRepositoryTool._run()` validates `clone_path` with `resolved.is_relative_to(basepath)`, which still allows `resolved == basepath`. That permits cloning/initializing a git repo directly in the shared root (e.g. `/git-repos`), risking workspace contamination and git safety/config failures.

## Issue Context
- The tool’s branch flow uses `git init` + `git checkout` in `cwd=clone_path`, which will write a working tree directly into that directory.
- The MCP container config only marks `/git-repos/*` (and `/git-repos/applicability/*`) as safe git directories; `/git-repos` itself is not included.

## Fix Focus Areas
- ymir/tools/privileged/gitlab.py[466-470]
- ymir/tools/privileged/tests/unit/test_gitlab.py[233-276]

## What to change
1. After resolving/normalizing the path, require `resolved` to be a **strict** descendant of `basepath` (e.g., `if resolved == basepath: raise ToolError(...)`).
2. Add a unit test asserting `/git-repos` (or `mock_git_repo_basepath` itself) is rejected with a clear ToolError message.

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


Grey Divider

Qodo Logo

Comment thread ymir/tools/privileged/gitlab.py

@nforro nforro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@opohorel
opohorel merged commit 8ce7385 into packit:main Jul 9, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants