chore(devex): route system-tables test fixtures through product doors - #91460
Conversation
test_system_tables.py reached sealed products' models with apps.get_model plus TYPE_CHECKING imports. That recipe was retired in #88974 because the registry lookup leaves no import edge: tach and import-linter cannot see the dependency, mypy cannot type it, and snob does not select the test when the model changes. The file kept teaching the pattern anyway, and agents copy the strongest local precedent (an agent-driven stack reproduced it days after the docs flip). Replace every apps.get_model call with a visible import: - customer_analytics rows now come from its facade testing door, which re-exports the product's test factories (extended with the missing creators) - error_tracking rows come from its facade testing door (extended with creators for rules, releases, fingerprints, assignments, symbol sets) - conversations EmailThread and tasks/canvas models are importable surface (unsealed product, and tach-exposed model carve-outs), so plain imports replace the registry lookups Claude-Session: https://claude.ai/code/session_01QRH4gDAW5uPPUL8V1wUj2j
|
😎 Merged successfully - details. |
🤖 CI report
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
…tables-facade-fixtures
Codex flagged the unscoped() escape hatch: the helper creates a row for exactly one team, and the testing door exists to model the sanctioned pattern, so it should use the fail-closed manager's for_team scope. Claude-Session: https://claude.ai/code/session_01QRH4gDAW5uPPUL8V1wUj2j
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17fad8afd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Pure test-infrastructure refactor (fixtures moved from ad-hoc apps.get_model calls to typed facade functions); no production code path is touched, tests still pass, and CI is green. The one open Codex comment flags style (unscoped() vs for_team() on new customer_analytics factory helpers) but I verified in posthog/models/scoping/manager.py that both methods produce an identical inserted row when team_id is passed explicitly to .create() — there's no cross-team read/leak here, so it's a convention nit rather than a functional or security defect.
- Author wrote 3% of the modified lines and has 16 merged PRs in these paths (familiarity MODERATE).
- chatgpt-codex-connector[bot] reviewed the current head.
- Unresolved Codex P1 comment on products/customer_analytics/backend/test/factories.py suggests switching new helpers from .unscoped().create() to .for_team(team_id).create() for consistency with the fail-closed convention — verified non-blocking (create() sets team_id explicitly either way), but worth a follow-up cleanup for consistency.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 105L, 2F substantive, 578L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (578L, 4F, two-areas, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 17fad8a · reviewed head 17fad8a |
Same doctrine point Codex raised on the severity-rule creator: each helper writes a row for exactly one supplied team, so it should go through the fail-closed manager's for_team scope instead of the cross-team unscoped() escape hatch. The two pre-existing factories keep their shape to avoid churning code this PR did not add. Claude-Session: https://claude.ai/code/session_01QRH4gDAW5uPPUL8V1wUj2j
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
@chatgpt-codex-connector[bot] still has a review in flight (👀) — not approving over an unfinished review. The review re-runs on the next push, or re-request one once the reviewer finishes.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 105L, 2F substantive, 578L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (578L, 4F, two-areas, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 18e841d · reviewed head 18e841d |
|
/trunk merge |
There was a problem hiding this comment.
Approved.
Diff is confined to test factories and test-support "facade/testing" modules used only by test suites; it swaps raw apps.get_model()/TYPE_CHECKING lookups for typed factory imports with no production behavior change, matching the description exactly. No data models, migrations, APIs, auth, billing, or CI/build config are touched, and the latest commit received a clean Codex review (👍, no findings) with no unresolved comments or holds.
- Author wrote 3% of the modified lines and has 16 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from chatgpt-codex-connector[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 105L, 2F substantive, 578L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (578L, 4F, two-areas, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 18e841d · reviewed head 18e841d |
1 participant
Problem
test_system_tables.pycopy the retiredapps.get_modelfixture recipe, because the file still uses it. A review of an agent-authored stack reproduced the pattern days after chore(devex): drop the get_model recipe for core test fixtures #88974 removed the recipe from the docs.Changes
test_system_tables.pynow seeds sealed products' rows through their facade testing doors and imports every other product's models directly, so the file teaches only sanctioned patterns.create_issueaccepts a severity.EmailThreadmodels and the tasks/canvas models are imported directly. conversations is unsealed, and the tasks/canvas model classes are tach-exposed carve-outs, so plain imports are their sanctioned surface.How did you test this code?
test_system_tables.pysuite plus every pre-existing consumer of both testing doors (conversations, metrics) passes locally.hogli lint:tach,lint-imports,hogli product:lint --all, and repo-wide mypy (uv run mypy --cache-fine-grained .) are clean locally.Automatic notifications
Docs update
None. The doctrine text landed in #88974; this aligns the code with it.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
hogli reviewno longer exists (chore(devex): remove the greptile cli review, keep the skill #90504 removed it), so the local pass was /simplify's four-angle review. Applied: severity-rule creator switched fromfor_teamtounscoped().createto drop a redundant team lookup, plus a fail-closed comment and a docstring fix. Rejected: reintroducingTYPE_CHECKINGmodel imports for wrapper annotations (that shape is what this PR removes; mypy infers the types from the typed factories) and collapsing the customer_analytics two-file door (re-exporting the product's test factories is its existing convention, with outside consumers).