elastic / elastic/ai-github-actions

[framework-best-practices] Use pytest tmp_path and configurable self-review dir to avoid shared /tmp state

Open
#673 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
11
Forks
16
Avg merge
22h 9m
Merged PRs (30d)
31

Description

## Framework / Library Best Practices Findings

### 1. Make self-review artifact paths test-isolated with `pytest` fixtures
**Library:** `pytest>=8.0` (from `pyproject.toml` optional deps)

**Library feature:** `tmp_path` / `tmp_path_factory` (per-test isolated filesystem), plus `monkeypatch` for path injection.

**Current code:**
- `tests/test_safe_input_ready_to_make_pr.py:183,206,215,268-269,278,286-287,301,311` reads/writes assertions against fixed global paths under `/tmp/self-review/...`.
- `.github/workflows/gh-aw-fragments/safe-output-create-pr.md:71-77,124,131` writes review artifacts to hardcoded `/tmp/self-review/` paths.
- `.github/workflows/gh-aw-fragments/safe-output-push-to-pr.md:77-82,137,145` does the same.
- `github/workflows/gh-aw-fragments/safe-output-push-to-pr.md:62-65,73` mirrors the same hardcoded path pattern.

**What is wrong:** Test coverage relies on process-global `/tmp/self-review` state rather than per-test temporary directories, and the production snippets hardcode that path with no override. This bypasses pytest’s native isolation primitives and makes test state global by design.

**Why it matters (medium):** Global temp paths make tests non-hermetic and order-dependent, increasing flakiness risk across retries/parallelism and making local/CI behavior harder to reason about as test suites grow.

**Simplification:**
- In the Python snippets in both fragment families, introduce a configurable base directory (for example `SELF_REVIEW_DIR = os.environ.get("SELF_REVIEW_DIR", "/tmp/self-review")`) and use it for all artifact paths.
- In tests, set `SELF_REVIEW_DIR` to a per-test `tmp_path` via `monkeypatch`, then assert against files in that isolated directory.
- Keep `/tmp/self-review` as the runtime default to preserve existing behavior outside tests.

**Documentation:**
- (docs.pytest.org/redacted)
- (docs.pytest.org/redacted)

## Suggested Actions
- [ ] Add a configurable self-review artifact directory variable in `safe-output-create-pr.md` and both `safe-output-push-to-pr.md` fragments, defaulting to `/tmp/self-review`.
- [ ] Update `tests/test_safe_input_ready_to_make_pr.py` to use `tmp_path` + `monkeypatch` instead of hardcoded `/tmp/self-review` assertions.
- [ ] Run `make test` to verify behavior is unchanged and tests remain green with isolated temp directories.

---
[What is this?](https://ela.st/github-ai-tools) | [From workflow: Trigger Framework Best Practices](https://github.com/elastic/ai-github-actions/actions/runs/23004440596)

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.