Repository navigation
chore(ci): stop new files in ee/ without human approval - #124071
Conversation
|
😎 Merged successfully - details. |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 3f5d4f9: 2 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
🤖 CI report🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong. ✅ Duplication (Python) — cleanNew Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying. ✅ Duplication (TypeScript) — cleanNew TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying. |
ReeceJones
changed the title
|
Risk: No findings Follow-up increment adds the guard's tests at Sentinel reviewed |
| # file that a content scan cannot. Exemptions live in the script. | ||
| - name: Check for new files under ee/ | ||
| if: github.event_name == 'pull_request' | ||
| run: python .github/scripts/check-no-new-ee-files.py --against HEAD^1 --committed |
There was a problem hiding this comment.
P1 · Gate the script invocation for branches that predate it
When this workflow reaches an unrebased PR whose checkout lacks the new script, python exits with a missing-file error. repo-checks then marks that failure deterministic and cancels Backend CI for unrelated changes. AGENTS.md:139 explicitly requires compatibility with these branches; gate the invocation on script availability.
| - name: Check for new files under ee/ | ||
| if: github.event_name == 'pull_request' | ||
| run: python .github/scripts/check-no-new-ee-files.py --against HEAD^1 --committed |
There was a problem hiding this comment.
P2 · Run the guard for every added path under ee/
A PR adding only ee/new.md or ee/frontend/new.ts skips repo-checks: its backend filter excludes Markdown and ee/frontend/**, and neither path triggers the other job conditions. The new guard never runs, bypassing the all-file approval requirement in ee/AGENTS.md:3–5; add an independent filter covering every ee/** path.
webjunkie
left a comment
webjunkie
left a comment
There was a problem hiding this comment.
🤖 Agent review · steered by a person
Requesting changes. The human-approval step is not enforced yet. Where this check should live is still open.
Make the exemption list need a human
The EXEMPT set sits in .github/scripts/check-no-new-ee-files.py, and owners.yaml does not mark that file sensitive. An agent can add a path there, and stamphog can approve the PR. Add the script to the sensitive team-devex block in owners.yaml, or move the list to a data file that is routed there.
Drop the semgrep README edit
It belongs to the semgrep attempt that the PR replaced. It also pulls the "To flip a rule" paragraph into the bullet list above it.
Move the test next to the script
posthog/test/repo_invariants/ is for tests that scan the whole repository. This test checks one script against a temporary git repo. Tests for .github/scripts live next to them as test_*.py and run in ci-scripts.yml.
Open question: where the check lives
A path comparison is the right approach. Semgrep cannot see an empty file, and a repo invariant would need a baseline of every file in ee/. But this adds a standalone script, its own CI step and its own preflight entry. #124050 does the same for a different guard. A shared home for diff-based guards that need a human sign-off may fit better. That is not decided yet, so no change is needed here for now.
- owners.yaml: the script that holds EXEMPT is sensitive team-devex, so a human reviews an exemption. - Move the guard's test to .github/scripts, where ci-scripts.yml runs it. - Drop the .semgrep/rules/devex/README.md edit, which belonged to the replaced semgrep attempt.
webjunkie
left a comment
webjunkie
left a comment
There was a problem hiding this comment.
🤖 Agent review · steered by a person
Approving.
Problem
eeis deprecated but we dont have any guards to prevent new files from being added.Changes
A path check,
.github/scripts/check-no-new-ee-files.py, fails a pull request that adds a file underee/. It runs in CI (Repo checksin Backend CI) and inhogli ci:preflight, and it compares paths, so an empty__init__.pycounts too. This replaces the semgrep rule the first commit tried, which could not see an empty file.A developer exempts one file by adding its exact path to
EXEMPTin that script, so the exception is a line a human approves in review.ee/AGENTS.mdtells an agent not to add a file there without explicit human approval.How did you test this code?
This PR
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted) - or - Fully autonomous
Agent: tool, model