chore(hogli): validate canonical facade alternation in tach.toml#56699
Merged
chore(hogli): validate canonical facade alternation in tach.toml#56699
Conversation
webjunkie
changed the title
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: common/hogli/product/checks.py
Line: 71
Comment:
**Backslash form not normalised in `from_patterns` comparison**
`from_patterns` contains raw TOML strings extracted by `re.findall(r'"(.*?)"', ...)`, so if any per-product legacy-leak block uses the escaped form `"products\\.experiments"` the extracted string will be `products\\.experiments`, which will never equal the plain `module_path` string `products.experiments`. The block would be silently skipped, causing `has_legacy_interface_leaks` to return `False` when it should return `True`.
`_pattern_targets_public_surface` correctly calls `.replace("\\", "")` before comparing — the same normalisation is needed here:
```python
if from_patterns != [module_path]:
```
should be
```python
normalized_from = [p.replace("\\", "") for p in from_patterns]
if normalized_from != [module_path]:
```
```suggestion
normalized_from = [p.replace("\\", "") for p in from_patterns]
if normalized_from != [module_path]:
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: common/hogli/tests/test_checks.py
Line: 582-705
Comment:
**`TestValidateFacadeAlternation` is not parametrised**
The repo rule is "we always prefer parametrised tests." The nine methods in `TestValidateFacadeAlternation` share the same shape — build a filesystem with `_mkproduct`, feed a tach string to `validate_facade_alternation`, and assert on the returned issues. They could be expressed as a single `@pytest.mark.parametrize` table where each row carries the products to create (name, isolated flag) and the TOML string, greatly reducing boilerplate and making it easier to add cases. `TestNamesFromPattern` in the same PR is already parametrised and shows the pattern works well here too.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "refactor(hogli): share interface-block p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89638b6b3b
ℹ️ 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".
Comment on lines
+160
to
+163
| if not is_isolated_product(product_dir / "backend"): | ||
| issues.append( | ||
| f"canonical facade alternation lists '{name}' but products/{name}/backend/facade/contracts.py " | ||
| "is missing — either add contracts.py or remove the entry from tach.toml" |
There was a problem hiding this comment.
This new isolation gate is now a hard error, but the current repository state already violates it: tach.toml's canonical alternation still includes platform_features while products/platform_features/backend/facade/contracts.py is absent, so this branch will always emit an issue here and then fail product:lint --all (which is run in repo checks at .github/workflows/ci-backend.yml, line 497). That makes backend CI fail for unrelated PRs immediately after this commit; the change should either update tach.toml in the same commit or keep these findings non-blocking until existing entries are reconciled.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new global Hogli lint that validates the canonical [[interfaces]] facade alternation in tach.toml, ensuring isolated products are correctly and consistently exposed via their public surface.
Changes:
- Introduces
validate_facade_alternation()and supporting helpers to parse/validate canonical[[interfaces]]blocks and product coverage. - Integrates the new validation into
lint_all_products()with GitHub Actions annotations. - Adds unit tests for alternation validation and
from-pattern name extraction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| common/hogli/product/checks.py | Adds helpers to parse [[interfaces]] blocks, classify canonical public-surface exposes, extract product names from from patterns, and validate alternation completeness/staleness. |
| common/hogli/product/lint.py | Runs the new global tach.toml alternation validation once per --all invocation and surfaces results (incl. GH annotations). |
| common/hogli/tests/test_checks.py | Adds coverage for alternation validation scenarios and _names_from_pattern() parsing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Size Change: 0 B Total Size: 133 MB ℹ️ View Unchanged
|
`hogli product:lint --all` now flags stale and missing entries in the canonical `[[interfaces]]` block (`backend.facade.* + presentation.views.*`): 1. names listed but `products/<name>/` does not exist 2. names listed but `backend/facade/contracts.py` is missing 3. products with `backend/facade/contracts.py` not listed in the alternation Per-product `TachCheck` already covered (3) via substring search but did not catch (1) or (2), so stale alternation members were never surfaced. This commit reports two real gaps in the current tach.toml (platform_features listed without contracts.py; legal_documents isolated but absent from the alternation) without auto-fixing them.
…tion checks
`has_legacy_interface_leaks` and `validate_facade_alternation` were both
iterating `[[interfaces]]` blocks and re-implementing the
"expose pattern targets backend.facade or backend.presentation" check —
with divergent backslash semantics (the old `startswith("backend\\.facade")`
literal happened to work only because the on-disk two-backslash form never
matched, and legacy blocks have no facade exposes anyway).
Both now go through `_iter_interface_blocks` for parsing and
`_pattern_targets_public_surface` for the public-surface check. The new
helper normalizes backslashes so it's correct for both the on-disk form
(`\\.`) and Python-string fixtures (`\.`).
The canonical `[[interfaces]]` alternation in tach.toml drifted from the actual product set on disk. The new product:lint validator surfaced two gaps: - `platform_features` was listed but has no `backend/facade/contracts.py`, so the alternation entry could not be exercised — remove it. - `legal_documents` has `backend/facade/contracts.py` but was absent from the alternation, so its facade was not reachable through the canonical public surface — add it. Also reorder the remaining entries alphabetically so future drift is easier to spot.
- `has_legacy_interface_leaks`: normalize backslashes in `from_patterns` before comparing to `module_path`, matching the normalization used in `_pattern_targets_public_surface` and `_names_from_pattern`. Without this, a hypothetical legacy block written as `from = ["products\\.X"]` would be silently skipped (greptile P1). - Tests: collapse the nine `TestValidateFacadeAlternation` methods into one parametrized test row table, matching the repo convention used by the neighbouring `TestNamesFromPattern` and `TestLegacyInterfaceLeaks` classes (greptile P2, AGENTS.md "prefer parametrized tests"). - Tests: clarify the on-disk-form comment so it correctly describes `\\\\` source becoming two literal backslashes at runtime (Copilot).
Adding `legal_documents` to the canonical facade alternation restricts its public surface to `backend.facade.*` and `backend.presentation.views.*`, but core still imports `backend.admin.LegalDocumentAdmin`, `backend.models.LegalDocument`, and `backend.presentation.webhook.legal_document_pandadoc_webhook` directly. Without an explicit carve-out tach rejects those imports. Add a legacy-leak `[[interfaces]]` block declaring those three surfaces as permitted internal access — same pattern as experiments, mcp_store, and tracing. The block should shrink as core migrates to the facade. Per the convention documented above the legacy blocks, also remove the no-op `backend:contract-check` script from legal_documents/package.json so CI treats the product as non-isolated until the leaks are gone.
Removing `backend:contract-check` from `legal_documents/package.json` left `turbo.json` overriding inputs for a task that no longer exists, which `IsolationChainCheck` correctly flags as dead config. The override only contained the contract-check task, so delete the whole file — matching experiments/mcp_store/tracing, which have no turbo.json override either.
The previous commits added `legal_documents` to the canonical facade alternation (with a legacy-leak block, removing its contract-check script, and deleting its turbo.json) because the validator's rule 3 — "every product with backend/facade/contracts.py must be in the canonical alternation" — fired on it. That rule conflated "has facade scaffolding" with "ready for canonical exposure", which forced a real product into a transitional state it hadn't opted into. Drop the rule and restore legal_documents. The validator now only catches stale entries (rules 1 + 2): names listed that don't resolve to a real product, and names listed without backend/facade/contracts.py. The inverse direction is left to the per-product TachCheck (which already requires isolated products to appear in *some* [[interfaces]] block).
…ecks Extend product:lint --all with checks that validate tach.toml declarations themselves, not just what tach enforces at import time. New checks: - No mixed facade + internal expose in a single [[interfaces]] block (catches the pattern where someone adds backend.facade.* next to backend.models.* — making isolation decorative) - No overly broad expose patterns (backend.* matches everything) - Alphabetical sort of canonical alternation names (prevents merge conflicts) - Dangling [[interfaces]] from references (must point to existing [[modules]]) - Dangling [[modules]] depends_on entries (must reference existing modules)
webjunkie
force-pushed
the
claude/validate-product-lint-tach-mrtn3
branch
from
d30efb6 to
5ad726b
Compare
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
tools/hogli-commands/hogli_commands/tests/test_checks.py:895-918
**`TestAlternationSorting` should be parameterised**
Both methods follow the exact same fixture pattern — create the same products, build a TOML string via `_iface`, call `validate_facade_alternation`, and assert. Only the alternation order and the expected outcome differ. Per the team rule of always preferring parameterised tests, these should be expressed as a single `@pytest.mark.parametrize` table, consistent with `TestNamesFromPattern`, `TestValidateInterfaceBlocks`, and `TestValidateTachReferences` in the same file.
### Issue 2 of 2
tools/hogli-commands/hogli_commands/product/checks.py:186-192
**`_ordered_names_from_alternation` duplicates `_names_from_pattern`**
The first four lines of both functions are identical: normalize backslashes and apply the same `^products\.\(([^)]+)\)$` regex. This violates OnceAndOnlyOnce (simplicity rule 3). A single private helper that returns the `re.Match` or both representations would eliminate the duplication.
```python
def _parse_alternation_match(pattern: str) -> re.Match | None:
"""Return the regex match for an alternation pattern, or None."""
return re.match(r"^products\.\(([^)]+)\)$", pattern.replace("\\", ""))
```
`_names_from_pattern` and `_ordered_names_from_alternation` can then both call `_parse_alternation_match` instead of re-running the same normalization and regex.
Reviews (2): Last reviewed commit: "Merge branch 'master' into claude/valida..." | Re-trigger Greptile |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR exceeds the automated size ceiling (541 lines). The substantive P1 concern about platform_features causing a hard CI failure appears addressed by the tach.toml change, but the complexity of the new facade-alternation validation logic warrants a human review to confirm correctness before merging.
rnegron
approved these changes
May 6, 2026
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
As the product architecture evolves toward isolated products with facade contracts, we need to ensure consistency in how these facades are exposed globally. Currently, there's no validation that:
backend/facade/contracts.py) are listed in the canonical facade alternation blockThis can lead to stale entries or missing facades that aren't reachable from outside the product.
Changes
Added comprehensive validation for the canonical
[[interfaces]]alternation block intach.toml:New functions in
checks.py:_iter_interface_blocks()— Helper to parse all[[interfaces]]blocks from tach.toml_pattern_targets_public_surface()— Check if a pattern targetsbackend.facadeorbackend.presentation_is_canonical_facade_expose()— Identify canonical blocks (those exposing only public surface)_names_from_pattern()— Extract product names from tach patterns (handlesproducts.nameandproducts.(a|b|c)forms)validate_facade_alternation()— Main validation function that enforces three rules:Integration in
lint.py:_lint_facade_alternation()to run the global check duringlint_all_products()Refactoring in
checks.py:has_legacy_interface_leaks()to use the new helper functions, reducing duplication and improving maintainabilityHow did you test this code?
Added comprehensive unit tests in
test_checks.py:TestValidateFacadeAlternation— 9 test cases covering:TestNamesFromPattern— 12 parametrized tests covering pattern extraction for various formsAll tests pass and validate the three core rules.
Publish to changelog?
No — this is internal tooling for product architecture validation.
https://claude.ai/code/session_01HPswQQ5v1x2MT16pcdSy2K