Skip to content

feat(build): Prune clones directory#6471

Open
Aradhya-Tripathi wants to merge 1 commit into
developfrom
prune-clone-dir
Open

feat(build): Prune clones directory#6471
Aradhya-Tripathi wants to merge 1 commit into
developfrom
prune-clone-dir

Conversation

@Aradhya-Tripathi
Copy link
Copy Markdown
Contributor

No description provided.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 20, 2026

Greptile Summary

This PR adds a prune_clone_directory operation that deletes the contents of /home/frappe/agent/.clones on build servers via a new Ansible playbook, mirroring the existing prune_docker_system pattern.

  • server.py gains prune_clone_directory (enqueues on the long queue) and _prune_clone_directory (runs the Ansible play); the exception log message incorrectly says \"Prune Build Directory Exception\" instead of \"Prune Clone Directory Exception\".
  • The Ansible role runs rm -rf /home/frappe/agent/.clones/* as root and then resets directory ownership to frappe:frappe; the top-level playbook specifies gather_facts: yes even though no facts are consumed.

Confidence Score: 3/5

The change is functionally correct but ships with a wrong log message that will make production debugging harder; the Ansible role also runs a destructive rm -rf as root when only the permission-reset step actually needs root.

The wrong log string in _prune_clone_directory means that if the playbook ever fails in production, the logged error will silently point to the wrong operation, making the failure harder to diagnose. The gather_facts: yes and the overly broad root privilege in the Ansible role are minor, but together they suggest the PR could benefit from one more pass before merging.

press/press/doctype/server/server.py (wrong log message) and press/playbooks/roles/prune_clones_directory/tasks/main.yml (rm -rf runs as root)

Important Files Changed

Filename Overview
press/press/doctype/server/server.py Adds prune_clone_directory public entry point and _prune_clone_directory worker; contains a misleading log message ("Prune Build Directory Exception") that should say "Prune Clone Directory Exception".
press/playbooks/prune_clones_directory.yml New top-level playbook; uses gather_facts: yes while no facts are needed, inconsistent with the sibling docker_system_prune.yml that uses gather_facts: no.
press/playbooks/roles/prune_clones_directory/tasks/main.yml Runs rm -rf /home/frappe/agent/.clones/* as root and then resets directory ownership to frappe:frappe; root is only strictly needed for the permission-reset step.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
press/press/doctype/server/server.py:2455-2457
The exception log message says `"Prune Build Directory Exception"` instead of `"Prune Clone Directory Exception"`. When this exception fires in production, searching logs for it will be misleading and point to the wrong operation.

```suggestion
		except Exception:
			log_error("Prune Clone Directory Exception", doc=self)
			if throw_on_failure:
```

### Issue 2 of 3
press/playbooks/prune_clones_directory.yml:6
`gather_facts: yes` collects host facts (OS, network, hardware) that are unused by this role's two tasks (a `rm -rf` and a `file` permission reset). The sibling `docker_system_prune.yml` uses `gather_facts: no` for the same reason — it adds unnecessary overhead per invocation.

```suggestion
  gather_facts: no
```

### Issue 3 of 3
press/playbooks/roles/prune_clones_directory/tasks/main.yml:3
**`rm -rf` with shell glob runs as root**

The `command` module runs `rm -rf /home/frappe/agent/.clones/*` as root (via `become_user: root`). The files inside `.clones` are owned by `frappe`, so root is not strictly required for deletion — only the permission-reset step needs it. Consider dropping privileges for this task by adding `become: no` or `become_user: frappe` so the wildcard expansion and deletion run as the file owner rather than root.

Reviews (1): Last reviewed commit: "feat(build): Prune clone directory on bu..." | Re-trigger Greptile

Comment on lines +2455 to +2457
except Exception:
log_error("Prune Build Directory Exception", doc=self)
if throw_on_failure:
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.

P1 The exception log message says "Prune Build Directory Exception" instead of "Prune Clone Directory Exception". When this exception fires in production, searching logs for it will be misleading and point to the wrong operation.

Suggested change
except Exception:
log_error("Prune Build Directory Exception", doc=self)
if throw_on_failure:
except Exception:
log_error("Prune Clone Directory Exception", doc=self)
if throw_on_failure:
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/server/server.py
Line: 2455-2457

Comment:
The exception log message says `"Prune Build Directory Exception"` instead of `"Prune Clone Directory Exception"`. When this exception fires in production, searching logs for it will be misleading and point to the wrong operation.

```suggestion
		except Exception:
			log_error("Prune Clone Directory Exception", doc=self)
			if throw_on_failure:
```

How can I resolve this? If you propose a fix, please make it concise.

hosts: all
become: yes
become_user: root
gather_facts: yes
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.

P2 gather_facts: yes collects host facts (OS, network, hardware) that are unused by this role's two tasks (a rm -rf and a file permission reset). The sibling docker_system_prune.yml uses gather_facts: no for the same reason — it adds unnecessary overhead per invocation.

Suggested change
gather_facts: yes
gather_facts: no
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/playbooks/prune_clones_directory.yml
Line: 6

Comment:
`gather_facts: yes` collects host facts (OS, network, hardware) that are unused by this role's two tasks (a `rm -rf` and a `file` permission reset). The sibling `docker_system_prune.yml` uses `gather_facts: no` for the same reason — it adds unnecessary overhead per invocation.

```suggestion
  gather_facts: no
```

How can I resolve this? If you propose a fix, please make it concise.

@@ -0,0 +1,11 @@
---
- name: Prune Clones Directory
command: rm -rf /home/frappe/agent/.clones/*
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.

P2 rm -rf with shell glob runs as root

The command module runs rm -rf /home/frappe/agent/.clones/* as root (via become_user: root). The files inside .clones are owned by frappe, so root is not strictly required for deletion — only the permission-reset step needs it. Consider dropping privileges for this task by adding become: no or become_user: frappe so the wildcard expansion and deletion run as the file owner rather than root.

Prompt To Fix With AI
This is a comment left during a code review.
Path: press/playbooks/roles/prune_clones_directory/tasks/main.yml
Line: 3

Comment:
**`rm -rf` with shell glob runs as root**

The `command` module runs `rm -rf /home/frappe/agent/.clones/*` as root (via `become_user: root`). The files inside `.clones` are owned by `frappe`, so root is not strictly required for deletion — only the permission-reset step needs it. Consider dropping privileges for this task by adding `become: no` or `become_user: frappe` so the wildcard expansion and deletion run as the file owner rather than root.

How can I resolve this? If you propose a fix, please make it concise.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

❌ Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.29%. Comparing base (5ae5dec) to head (fbf8b10).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
press/press/doctype/server/server.py 14.28% 12 Missing ⚠️

❌ Your patch check has failed because the patch coverage (14.28%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6471      +/-   ##
===========================================
+ Coverage    49.69%   56.29%   +6.60%     
===========================================
  Files          953      953              
  Lines        78831    78845      +14     
  Branches       360      500     +140     
===========================================
+ Hits         39173    44387    +5214     
+ Misses       39634    34429    -5205     
- Partials        24       29       +5     
Flag Coverage Δ
dashboard 89.54% <ø> (+29.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant