chore(devex): stop honoring contract-check on facades that re-export internals - #71127
Closed
webjunkie wants to merge 16 commits into
Closed
chore(devex): stop honoring contract-check on facades that re-export internals#71127webjunkie wants to merge 16 commits into
webjunkie wants to merge 16 commits into
Conversation
…internals
turbo-discover treats a product as isolated when package.json declares
backend:contract-check, and skips the whole Django suite when only that
product changed. The skip assumes a change inside the product can only break
the product's own tests. tach proves the import half of that, but a facade
that re-exports its own classes hands them to core through the one legal
import, so the behavior core depends on sits under logic/ - outside the
contract-check inputs (facade/**, presentation/**). It can then change while
facade/** stays byte-identical and the suite is skipped anyway.
demo is the live case: facade/api.py re-exports MatrixManager from
logic/matrix/manager.py, posthog/api/signup.py drives
ensure_account_and_save on it, and no in-product test pins that method. Its
contracts.py is empty and says so, so the "contracts changed -> run Django"
tripwire can never fire either.
Drop the script and the now-dead turbo.json override from the three products
whose facades advertise classes (demo, endpoints, streamlit_apps). They fall
back to the full suite, which is the conservative side.
Add the leak to the lint so the state is visible and can't silently come
back:
- warn when a facade advertises a class or constant it doesn't own
- fail only if a product re-adds contract-check while still leaking, which
no product does today, so master stays green
- feed it through eligible_for_isolated_tests so the inverse nag ("missing
backend:contract-check") stops pushing leaky products into claiming the
skip
Functions are allowed: one entry point with one signature keeps "behavior
tests live in-product" checkable. Constants belong in facade/enums.py and
data/error types in facade/contracts.py - both already contract-check inputs.
Keyed on __all__, the advertised surface. Treating every public binding as
exported flags the helpers a real facade imports in order to call them, which
would demote visual_review and error_tracking.
pr-assigner-resolver-posthog
Bot
requested review from
a team,
MattBro,
fercgomes and
rafaeelaudibert
and removed request for
a team
July 15, 2026 15:50
Contributor
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 64.75 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.3 KiB | ../node_modules/.pnpm/posthog-js@1.402.2/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 235.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 222.9 KiB | ../node_modules/.pnpm/posthog-js@1.402.2/node_modules/posthog-js/dist/module.js |
| 164.0 KiB | src/queries/validators.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.8 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | ../packages/quill/packages/quill/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Dist folder size — no change
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1312.24 MiB · no change
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens the safety of “isolated product” test skipping by removing backend:contract-check from products whose facades leak implementation details, and by adding a hogli product:lint check that detects (some) facade re-exports that would make the skip unsound.
Changes:
- Demotes
demo,endpoints, andstreamlit_appsby removingbackend:contract-checkscripts and deleting theirturbo.jsoncontract-check input narrowing overrides. - Adds facade re-export detection (
get_facade_reexports) and threads it into isolation eligibility (eligible_for_isolated_tests) plusproduct:lintmessaging. - Adds pytest coverage for the new detector and its interaction with isolation eligibility.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/hogli-commands/hogli_commands/tests/test_checks.py | Adds TestFacadeReexports coverage for the new facade re-export detector and isolation eligibility behavior. |
| tools/hogli-commands/hogli_commands/product/isolation.py | Extends IsolationStatus with facade_reexports and uses it to withhold isolated-test eligibility when the facade leaks. |
| tools/hogli-commands/hogli_commands/product/checks.py | Surfaces warnings/issues when a facade re-exports leaked names, and improves messaging formatting. |
| tools/hogli-commands/hogli_commands/product/ast_helpers.py | Introduces AST-based detection of re-exported symbols advertised via __all__ in facade/api.py. |
| products/streamlit_apps/turbo.json | Removes now-dead contract-check input narrowing config. |
| products/streamlit_apps/package.json | Removes backend:contract-check script so the product no longer claims isolation. |
| products/endpoints/turbo.json | Removes now-dead contract-check input narrowing config. |
| products/endpoints/package.json | Removes backend:contract-check script so the product no longer claims isolation. |
| products/demo/turbo.json | Removes now-dead contract-check input narrowing config. |
| products/demo/package.json | Removes backend:contract-check script so the product no longer claims isolation. |
| products/demo/backend/facade/contracts.py | Updates module docstring to explicitly warn readers not to copy the pattern and clarifies isolation implications. |
| products/data_catalog/backend/facade/contracts.py | Updates module docstring to explicitly warn readers not to copy the pattern and clarifies isolation implications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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.
Two problems the review bots caught in the first pass. Lazy facades were invisible. data_modeling and data_warehouse advertise their surface with `__all__ = sorted(_LAZY)` and resolve names in __getattr__ (PEP 562), which keeps heavy logic modules off the django.setup() path and breaks an import cycle with warehouse_sources. The detector only read a list literal and an ImportFrom source map, so both read as clean while carrying backend:contract-check - the exact state the check exists to catch. The _LAZY dict is the import map for that shape, so read it. The class rule was too blunt. "Anything that isn't a function" would have demoted data_modeling over `class HasDependentsError(Exception): pass` and streamlit_apps over five ints and two exception markers. The concern is a caller driving methods the product never treated as its API, and an error marker or a plain data class has none. So only a class that carries behavior (public methods, or a non-exception base that has them) disqualifies the skip; a misplaced type or constant warns instead. Net effect on who pays the full Django suite: - data_warehouse joins demo and endpoints (WebhookS3Sink has run(), HogQLQueryFixerTool extends MaxTool) - streamlit_apps keeps contract-check - its facade is real, the leak was AppRuntimeError/AppRuntimeConcurrencyError and some ints - data_modeling and warehouse_sources keep it, warned only
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Note 🤖 stamphog reviewed Gates denied this PR (oversized/two-area tier failure), and separately two Codex review comments raise unresolved, substantive gaps in the new facade-leak detector (it only scans facade/api.py and doesn't treat non-class re-exports as leaks) that the author hasn't addressed on the current head.
Gate mechanics and policy version
|
…ades Reverses the demotions. Codex was right that scanning only facade/api.py missed most of the surface: a product's public surface is the whole backend/facade/ package (tach exposes backend.facade.*), and capability submodules like queries.py and temporal.py are what core registers and dispatches on. Scanning api.py alone missed 6 more products, and moving a class into a sibling module would have evaded the check in one line. With every facade module scanned, the earlier rule would have demoted customer_analytics, data_modeling, error_tracking, metrics, tasks and warehouse_sources - including the products architecture.md holds up as correct. That is a reductio: re-exporting a class core dispatches on is the house pattern, not a defect. The defect is only ever re-export WITHOUT input coverage. warehouse_sources and error_tracking already settle it the sound way, and tach.toml spells out the invariant: keep the module in the contract-check inputs so any change to it still re-runs the Django suite. So the rule is now: a re-exported behavioral class whose defining module no input watches disqualifies the skip. The remedy is a turbo.json glob, not demotion. Widening turns out to be cheap - measured share of each backend now watched: warehouse_sources 6%, engineering_analytics 11%, visual_review 15%, data_warehouse 18%, endpoints 20%, demo 22%, data_modeling 23%, streamlit_apps 24%, metrics/wizard 30%, legal_documents 31%, error_tracking 32%, tasks 38%, customer_analytics 41%. So no product is demoted, all 14 keep contract-check, and the skip still pays on the majority of changes. A module defining a re-exported class also counts as extended surface for has_narrowed_turbo_inputs, for the same reason permanent modules do: core holds the class through the legal facade import, so watching its module is what makes the skip sound and must not cost the narrowing.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…tics Two holes in the facade-leak detector, both of which let an unsound skip through the gate. Aliased re-exports resolved to the wrong symbol. `from ..logic.runner import Runner as PublicRunner` with `__all__ = ["PublicRunner"]` recorded only the public alias, so the definition walk looked for a class named PublicRunner, found nothing, and reported a behavioral class as an unlocatable constant - which the coverage check then waves through. The re-export map now carries the original symbol alongside the advertised name. This is live: products/ experiments aliases an ORM model as ExperimentModel, and products/tracing aliases functions. Glob matching was more permissive than Turbo. Stripping a pattern at its first `*` treated `backend/logic/*.py` as covering `backend/logic/deep/runner.py`, but Turbo's spec has `*` match within one path segment and `**` cross segments, so contract-check would not re-run on the nested change the lint had just accepted as watched. Compile the glob instead. No product uses a non-recursive input today, so this is a latent hole rather than a live one - but it is a gate, so it should not be looser than the thing it gates.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…lobs Two more holes in the facade-leak detector. Re-exports resolved by bare name across the whole backend, so a duplicate name picked whichever file sorted first. data_modeling is the live case and its own facade docstring spells out the trap: NodeType is models.node.NodeType via facade.models, while the distinct resolver enum models.modeling.NodeType goes out via facade.modeling. Same for DAG (models/dag.py vs models/modeling.py). Half of those resolved to the wrong implementation and read coverage off the wrong file. Imports now carry the module they came from, and that module is consulted first; the whole-backend scan stays as the fallback for re-export chains, where the immediate source (facade/models.py) defines nothing itself. The lookup also walked nested nodes, so a method could answer for a module-level name - data_catalog's `certify` matched the viewset action rather than the logic function. Module-level definitions only. And _input_targets_reexport let a whole-backend glob count as coverage just because it covers the re-exported module. That handed back the narrowing the broad-glob rule exists to reject: backend/** would have reported the skip as ON. A whole-backend glob is never a narrowing, whatever it happens to contain.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Two more spots where the coverage check was looser than Turbo, so the lint could call a re-exported class watched for a change that will not re-run backend:contract-check. A globstar swallowed the following slash, compiling backend/**/runner.py to ^backend/.*runner.py$ and matching backend/foo_runner.py. The slash is a segment boundary, so it spans whole segments now: backend/runner.py and backend/a/b/runner.py match, backend/foo_runner.py does not. Negated inputs were dropped before the coverage test, so backend/logic/** paired with !backend/logic/runner.py reported Runner as watched while Turbo excludes it - precisely the unsoundness the guard exists to catch. Exclusions are applied in order now, later entries winning, as Turbo does. They stay excluded from the narrowing surface test, which is unchanged. Neither is reachable today (no product uses a globstar-with-suffix or a negated contract-check input), but this gate should not be looser than the thing it gates.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…export Keying candidates by advertised name collapsed duplicates, so when two facade modules export the same name from different modules only the later one was checked for coverage. This is the same data_modeling case as the source-module fix, one level up: facade/models.py advertises NodeType from models/node.py and facade/modeling.py advertises a distinct NodeType from models/modeling.py, and only models/node.py survived the merge. With precise per-file inputs, the dropped definition could be uncovered and still pass. Candidates are a list now, and definitions resolve per (name, source module) rather than per name, so both NodeType definitions are checked. Identical triples still collapse, so a name two modules re-export from the same place stays one entry.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6840faacb
ℹ️ 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".
The facade package's __init__.py is public surface — `from products.foo.backend.facade import Runner` is a legal import — but it was skipped along with contracts/enums, so a class exported only from there was invisible and its defining module never had to be watched. No product leaks through it today (error_tracking and experiments both re-export from .api/.contracts, which is facade-internal and already covered by scanning api.py), and nothing changed for any product. It closes the one-line evasion of moving the export into __init__.py.
webjunkie
marked this pull request as draft
July 15, 2026 19:36
2 tasks
Contributor
Author
|
Superseded by #71486. The diagnosis here was right, but the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
turbo-discovertreats a product as isolated when itspackage.jsondeclaresbackend:contract-check, and skips the whole Django suite when only that product changed. Perproducts/architecture.md, that skip is a claim: a change inside the product can only break the product's own tests. tach proves the import half — nobody reaches past the facade. The doc is explicit (lines 71–75) that it cannot prove the other half.A facade re-exports a class defined under
logic/ormodels/. The class travels out through the one legal facade import, so core holds it and drives its methods — but those methods live outside the contract-check inputs. They can change while the watched inputs stay byte-identical, and the suite is skipped anyway.tach checkandlint-importsboth pass on this and always will; they're import-graph tools and no import moves.This is live today.
products/metricsdeclarescontract-checkwith inputsfacade/** + presentation/** + routes.py,facade/queries.pyre-exportsMetricsQueryRunner, and it's defined inbackend/metrics_query_runner.py— unwatched. Core instantiates it inposthog/hogql_queries/query_runner.py.Changes
The rule: a re-exported behavioral class whose defining module no contract-check input watches disqualifies the skip. The remedy is a
turbo.jsonglob, not demotion —warehouse_sourcesanderror_trackingalready settle it that way, andtach.tomlstates the invariant: keep the module in the inputs so any change to it still re-runs the Django suite. This mirrors the existinguncovered_permanent_modulesdoctrine for the re-export channel.Scoped deliberately:
api.py. The public surface is the wholebackend/facade/package (tach exposesbackend.facade.*);taskshas 18 facade modules,warehouse_sources11. Readingapi.pyalone would miss most of it and a one-line move to a sibling would evade the check.__all__ = sorted(_LAZY)with PEP 562__getattr__is howdata_modeling/data_warehousekeep heavy modules offdjango.setup(). The_LAZYdict is the import map for that shape.has_narrowed_turbo_inputs, for the same reason permanent modules do: watching it is what makes the skip sound, so it must not cost the narrowing.No product is demoted. All 14 keep
contract-check; nine widen their inputs. Widening is cheap — share of each backend now watched:So the skip still pays on the majority of changes everywhere.
Also documents
data_catalog's facade, whosecontracts.pyargues it needs no contracts because consumers are over HTTP — the test architecture.md line 83 names as wrong. It has nocontract-checkand the existinghas_real_facadecheck already withholds it.How did you test this code?
Automated, all run by me (the agent):
hogli product:lint --all→ all 67 pass. Asserted separately that nocontract-checkproduct has an uncovered class or lost its narrowing.hogli test tools/hogli-commands/hogli_commands/tests/→ 1197 pass.tach check→ all modules validated.lint-imports→ 1 kept, 0 broken. Both green before and after — that's the point, they can't see this.New tests:
TestFacadeReexportscovers detection across import styles, kinds, and the lazy-map shape;TestUncoveredFacadeClassescovers the widen-vs-flag rule (an uncovered class flags, a widened input clears it, an un-narrowed product has nothing uncovered). The regression they catch that nothing else did — if the detector stops matching absolute imports or lazy maps, a leaky facade reads as clean,eligible_for_isolated_testsflips true, and the Django suite gets skipped on changes core can observe. I shipped both of those bugs in earlier passes, hence the tests. Thedjango.db.models import Qcase guards the third-party false positive.No manual app testing — repo tooling, no runtime surface.
Automatic notifications
Docs update
products/architecture.mdpredates the facade capability-submodule pattern and still saysfacade/api.pyis "the only file other products are allowed to import" (line 191), which.claude/rules/product-isolation.mdandtach.tomlcontradict. Worth reconciling, but that's a doc decision rather than something to fold in here.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by Claude (Claude Code). Skills invoked:
/writing-tests,/simplify,/babysit-pr.The scope was corrected twice by review, both times because I'd generalised from too little evidence:
api.pyand demoteddemo/endpoints/streamlit_apps. Codex and Copilot both caught that lazy facades (__all__ = sorted(_LAZY)) were invisible, which hiddata_modelinganddata_warehouse— the exact state the check exists to catch.data_modelingoverclass HasDependentsError(Exception): pass, so the class rule was narrowed to behavioral classes only.api.pyisn't the whole facade. Scanning all modules showed the blanket rule demotes six products includingtasksanderror_tracking— a reductio proving re-export is the house pattern, and that the real defect is re-export without input coverage. That's the rule that landed, and it demotes nobody./simplifyseparately caught a real bug:"models" in sourcealso matcheddjango.db.models, which would have flagged every re-exportedQandPrefetch. Fixed by scoping to product-owned modules.