From 5e8242a985f71ea4f583661aa948e2e409d00887 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Thu, 13 Aug 2026 20:15:32 -0700 Subject: [PATCH 1/2] Add review instructions for build infra and root config --- .github/copilot-instructions.md | 53 +++++++++++++++++ .../instructions/build-config.instructions.md | 58 +++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 .github/copilot-instructions.md create mode 100644 .github/instructions/build-config.instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 000000000..e150ff1f5 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,53 @@ +# Review rules for all PRs + +## 0. What not to flag + +CI already runs deterministic gates on every PR. Do **not** spend review comments on anything they +decide. If a gate would catch it, the gate reports it, and a duplicate review comment is noise: + +| Gate | Covers | +| --- | --- | +| `precommit` (ruff, ruff-format, uv-lock, rstcheck) | Lint rules, formatting, import order, line length, `uv.lock` freshness, RST syntax | +| `typecheck` (pyright, strict) | Type errors and unused `type: ignore` in packages listed under `[tool.pyright] include` | +| `spellcheck` (codespell) | Spelling in code, docs, and comments | +| `shellcheck` | Shell script correctness in `scripts/*.sh` | +| `readme` | `README.rst` renders on PyPI | +| `docs` | Docs build | +| `generate` | The generated instrumentation README table being up to date | +| `lint-license-header-check` | Missing license headers | +| `oldest-deps-check` | Redundant pins in `tests/requirements.oldest.txt`, packages missing an `oldest` requirements file | +| `changelog` workflow | Direct edits to `CHANGELOG.md`, changelog fragment presence | +| `test` matrix | Test failures across the `oldest`/`latest`/`conformance` envs | + +Review the things a gate cannot decide: whether the change is correct, whether it matches the +GenAI semantic conventions, whether it actually achieves what the PR claims, and whether it +weakens or bypasses a gate. Style opinions that ruff does not enforce are not review comments. + +Repo-wide config is `tox.ini`, `pytest.ini`, root `pyproject.toml`, `.codespellrc`, +`.pre-commit-config.yaml`, and `scripts/`. A change there affects every package, so: + +- **No developer-local paths.** Personal venv or scratch directory names must not be added to + shared configs. `.gitignore` already covers `venv*/`, `.venv*/`, and `.tox`; anything else + belongs in a local ignore or gets renamed to match those patterns. Flag entries that only make + sense on one contributor's machine. +- **Skip lists must not weaken a gate.** Excluding a bare directory name from a lint or license + check also excludes any real source directory that happens to share that name. Prefer + path-anchored patterns (`*/.tox/*`) over bare components (`.tox`). +- **Shell must be portable.** Commands in `tox.ini` and `scripts/*.sh` run on both Linux and + macOS. Flag GNU-only flags (e.g. `xargs --no-run-if-empty`, `sed -i` without a backup suffix, + `readlink -f`). + +## 2. Config that packages can shadow + +Root config does not automatically reach every package. Before approving a change to a root +setting, check whether packages override the same section in their own `pyproject.toml`: + +- `pytest.ini` is only read when pytest's config lookup does not stop at a package + `pyproject.toml` first. These packages define `[tool.pytest.ini_options]` and therefore ignore + root `pytest.ini` entirely: `anthropic`, `agno`, `crewai`, `smolagents`, `llama-index`, + `claude-agent-sdk`. A new root pytest setting must either be added to each of them, or those + blocks removed. +- The same applies to `[tool.ruff]` and `[tool.pyright]` overrides. + +Flag a root-config change whose stated goal ("fixes the warning everywhere", "applies to all +tests") is not actually achieved for the shadowing packages. diff --git a/.github/instructions/build-config.instructions.md b/.github/instructions/build-config.instructions.md new file mode 100644 index 000000000..ddd6ca1dc --- /dev/null +++ b/.github/instructions/build-config.instructions.md @@ -0,0 +1,58 @@ +--- +applyTo: "{tox.ini,pytest.ini,pyproject.toml,uv.lock,.codespellrc,.pre-commit-config.yaml,scripts/**,.github/workflows/**,.github/actions/**}" +--- + +Review rules for PRs touching build, test-matrix, and CI configuration. Flag violations with a +link to the rule. See [copilot-instructions.md](../copilot-instructions.md) for the repo-wide +rules on shared config, portability, and shadowed settings, and for the gates whose findings must +not be repeated as review comments. + +These rules cover what CI cannot check: a test env that is never run, a bound that is never +exercised, a gate that silently stops covering ground. All of them pass CI while being wrong. + +## 1. Test matrix completeness + +A new package under `instrumentation//` must be wired into `tox.ini` in full. Check each of +these, not just the first: + +- `envlist`: `py3{…}-test-instrumentation-genai--{oldest,latest}`, the matching + `-conformance` entry, and `lint-instrumentation-genai-`. +- `[testenv] deps`: the factor-conditional `-r …/tests/requirements..txt` lines plus + `{[testenv]test_deps}` / `{[testenv]pytest_deps}`. Requirements install here, **not** in + `commands_pre`. +- `[testenv] commands`: the pytest line (which must `--ignore` `tests/test_conformance.py`), the + separate `-conformance` pytest line, and the `lint-…` ruff line. +- `[testenv:typecheck] deps`: `{toxinidir}/instrumentation/[instruments]`. + +The uv workspace picks up new packages via the `instrumentation/*` glob in root `pyproject.toml`, +so no edit is needed there. + +## 2. Lower bounds + +The `oldest` factor sets `UV_RESOLUTION=lowest-direct`, so lower bounds come from +`pyproject.toml` and `oldest-deps-check` already enforces that invariant. Do not re-report what +it catches. + +What is left to review is whether a changed bound is justified: a raised floor needs a reason in +the PR description (a feature or fix the code now depends on), and a lowered floor needs evidence +that the older version actually works, not just that the env resolves. + +## 3. Pyright scope + +`[tool.pyright] include` is opt-in and grows one package at a time. Typing more code is the goal, +so review changes here by direction: + +- Adding a package to `include` is welcome. `src/**` is never excluded. +- Excluding a package's `tests/**` and `examples/**` is the current convention when it joins + `include`, but only as a stopgap (see the comment above `exclude`). A PR that types its tests + and drops the exclusion is an improvement. +- Flag the reverse: a package removed from `include`, or a `src/**` path added to `exclude`, to + make a type error go away. + +## 4. Lint and check scripts + +A change that narrows what a check scans (new skip entry, new exclude path, a `|| true`) must say +why in the PR description. Silent scope reduction on a gate reads as "still passing" when it no +longer checks the same ground. + +See also [AGENTS.md](../../AGENTS.md) for general repo rules. From df945a484ffa3f6fae21ba97e6730ad859400969 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Thu, 13 Aug 2026 20:24:28 -0700 Subject: [PATCH 2/2] Fix section heading and applyTo glob syntax --- .github/copilot-instructions.md | 6 ++++-- .github/instructions/build-config.instructions.md | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index e150ff1f5..1b61066dc 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -2,8 +2,8 @@ ## 0. What not to flag -CI already runs deterministic gates on every PR. Do **not** spend review comments on anything they -decide. If a gate would catch it, the gate reports it, and a duplicate review comment is noise: +CI already runs deterministic gates on every PR. If a gate would catch it, the gate reports it, +and a duplicate review comment is noise: | Gate | Covers | | --- | --- | @@ -23,6 +23,8 @@ Review the things a gate cannot decide: whether the change is correct, whether i GenAI semantic conventions, whether it actually achieves what the PR claims, and whether it weakens or bypasses a gate. Style opinions that ruff does not enforce are not review comments. +## 1. Shared configuration + Repo-wide config is `tox.ini`, `pytest.ini`, root `pyproject.toml`, `.codespellrc`, `.pre-commit-config.yaml`, and `scripts/`. A change there affects every package, so: diff --git a/.github/instructions/build-config.instructions.md b/.github/instructions/build-config.instructions.md index ddd6ca1dc..90e2d7fef 100644 --- a/.github/instructions/build-config.instructions.md +++ b/.github/instructions/build-config.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: "{tox.ini,pytest.ini,pyproject.toml,uv.lock,.codespellrc,.pre-commit-config.yaml,scripts/**,.github/workflows/**,.github/actions/**}" +applyTo: "tox.ini,pytest.ini,pyproject.toml,uv.lock,.codespellrc,.pre-commit-config.yaml,scripts/**,.github/workflows/**,.github/actions/**" --- Review rules for PRs touching build, test-matrix, and CI configuration. Flag violations with a