-
Notifications
You must be signed in to change notification settings - Fork 17
♻️ refactor(arch): make ArchUnitTS architecture suite pass #394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
cteyton
wants to merge
14
commits into
main
Choose a base branch
from
architecture-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
79cf178
✅ test(architecture-tests): port ArchUnitTS suite onto main
cteyton 4b25f44
♻️ refactor(arch): move delayed-jobs interfaces to application layer
cteyton 3f92181
♻️ refactor(arch): hoist ICodingAgentDeployer port to @packmind/types
cteyton 3cbcff1
♻️ refactor(arch): inject command repositories via interfaces only
cteyton 20208b6
♻️ refactor(arch): move GithubAppMode type to @packmind/types
cteyton dd61c03
♻️ refactor(arch): introduce LlmRepositories aggregator
cteyton 7f1baa9
♻️ refactor(arch): render Claude plugins through ICodingAgentPort
cteyton 7ff81de
♻️ refactor(arch): hydrate package artefacts through domain ports
cteyton 3f466ed
👷 ci(arch): run architecture tests in CI and pre-push
cteyton 4cca5bb
🐛 fix(arch): track edition path files as arch cache inputs
cteyton d929658
🐛 fix(arch): sort packages with localeCompare
cteyton 2c61b37
📝 docs(arch): drop unimplemented domain-event rule from README
cteyton cffc6c3
👷 ci(arch): source PACKMIND_EDITION from repo variable
cteyton 43b86b2
🔧 chore(arch): drop PACKMIND_EDITION from test:arch script
cteyton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| name: Architecture Tests | ||
|
|
||
| # Runs the ArchUnitTS architecture rules (packages/architecture-tests) as a | ||
| # dedicated, NON-BLOCKING check. The job is allowed to fail (continue-on-error) | ||
| # so that pre-existing architecture violations are reported and monitored over | ||
| # time without blocking merges. Open the job logs to see the offending imports. | ||
|
|
||
| env: | ||
| HUSKY: '0' | ||
|
|
||
| on: | ||
| push: | ||
| branches: ['main'] | ||
| pull_request: | ||
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| architecture-tests: | ||
| name: Architecture rules (non-blocking) | ||
| runs-on: ${{ vars.ACTION_RUNNER_TAG || 'self-hosted' }} | ||
| timeout-minutes: ${{ github.actor == 'dependabot[bot]' && 15 || 30 }} | ||
| # Allow this job to fail without failing the workflow run or blocking PRs. | ||
| continue-on-error: true | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 1 | ||
|
|
||
| - uses: pnpm/action-setup@v4 | ||
|
|
||
| - name: Use Node.js from .nvmrc | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| cache: 'pnpm' | ||
|
|
||
| - name: Cache pnpm supply-chain verification | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.cache/pnpm | ||
| key: pnpm-verify-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }} | ||
|
|
||
| - run: pnpm install --frozen-lockfile --prefer-offline | ||
|
|
||
| # tsconfig.base.effective.json is git-ignored, so generate it before Nx | ||
| # builds its project graph (the vite/vitest plugins parse every project's | ||
| # tsconfig, which `extends` this file). Mirrors the quality workflow. | ||
| - name: Generate tsconfig | ||
| env: | ||
| PACKMIND_EDITION: ${{ vars.PACKMIND_EDITION }} | ||
| run: node scripts/select-tsconfig.mjs | ||
|
|
||
| # Generates tsconfig.arch.json and runs the dedicated `arch` Nx target, | ||
| # which re-reads PACKMIND_EDITION to pick the edition-aware tsconfig. | ||
| - name: Run architecture tests | ||
| env: | ||
| PACKMIND_EDITION: ${{ vars.PACKMIND_EDITION }} | ||
| run: pnpm run test:arch |
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| # architecture-tests | ||
|
|
||
| Executable enforcement of Packmind's hexagonal (ports & adapters) architecture, | ||
| powered by [ArchUnitTS](https://github.com/LukasNiessen/ArchUnitTS). | ||
|
|
||
| Each test is a rule about which layer may import which. Rules are expressed as | ||
| import-dependency constraints over the real TypeScript source graph (aliases | ||
| resolved via `tsconfig`), so a failure points at the exact offending file with a | ||
| clickable path. | ||
|
|
||
| ## Why a separate command | ||
|
|
||
| These tests are **not** part of `nx run-many -t test` / `npm run test:staged`. | ||
| They scan the whole monorepo's dependency graph (slower) and they intentionally | ||
| surface pre-existing violations as failures. Keeping them on a dedicated target | ||
| avoids breaking the main suite while still giving the team a runnable | ||
| architecture report. | ||
|
|
||
| ```bash | ||
| npm run test:arch | ||
| # = PACKMIND_EDITION=oss nx run architecture-tests:arch | ||
| ``` | ||
|
|
||
| The `arch` target first runs `scripts/build-arch-tsconfig.mjs`, which generates a | ||
| self-contained `tsconfig.arch.json` (git-ignored) from the edition-aware | ||
| `tsconfig.base.effective.json`. ArchUnitTS reads tsconfig with | ||
| `ts.readConfigFile` (which does not follow `extends`), so it needs an inlined, | ||
| include-bearing config — that is what the generator produces. | ||
|
|
||
| ## The workflow being enforced | ||
|
|
||
| ``` | ||
| NestJS controller → NestJS service → domain Adapter → UseCase | ||
| → application Service → repository interface (port) → repository implementation (infra) | ||
| ``` | ||
|
|
||
| Dependencies only ever point downward/inward. The rules below assert the | ||
| forbidden shortcuts and back-edges. | ||
|
|
||
| ## Rules | ||
|
|
||
| Rules live in `src/*.arch.spec.ts`. Layer selection globs are centralised in | ||
| `src/architecture.ts` (tolerant of folder-naming variants such as | ||
| `useCases`/`usecases` and `adapter`/`adapters`). | ||
|
|
||
| ### Core workflow layering — `src/layering.arch.spec.ts` | ||
|
|
||
| | Rule | Guards | | ||
| | ------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------ | | ||
| | `infra/repositories` ↛ `application/**` | Persistence must not depend upward on use cases/adapters/services. | | ||
| | `application/useCases` ↛ `infra/repositories` | Use cases reach data via services/ports, never concrete repositories. (`standard-use-case-architecture-patterns`) | | ||
| | `application/adapter(s)` ↛ `infra/repositories` | "Never directly call Repositories in Adapter classes." (`standard-use-case-architecture-patterns`) | | ||
| | `application/services` ↛ `infra/repositories` | Services depend on repository **interfaces** (`domain/repositories`), not implementations. | | ||
| | `application/services` ↛ `application/useCases` | Use cases orchestrate services, never the reverse — a service reaching back up inverts the layer. | | ||
| | `application/services` ↛ `application/adapter(s)` | The adapter is the port entry point at the top of the layer; services below it must not depend back up. | | ||
| | `apps/api` ↛ `infra/repositories` | No API file (controller, module, NestJS service) may reach concrete persistence — only ports. (`standard-nestjs-module-hierarchy`) | | ||
| | `apps/api` ↛ `application/**` | The API reaches each domain only through its `@packmind/types` port (injected by port-name via the HexaRegistry), never use cases/services/adapter directly. | | ||
| | `infra/schemas` ↛ `application/**` | EntitySchema files are pure ORM mapping. (`infra/jobs` legitimately wires application jobs; schemas do not.) | | ||
|
|
||
| ### Domain purity — `src/domain-purity.arch.spec.ts` | ||
|
|
||
| | Rule | Guards | | ||
| | ------------------------------ | -------------------------------------------------------------------- | | ||
| | `domain/**` ↛ `application/**` | The innermost ring must not know about the layer that depends on it. | | ||
| | `domain/**` ↛ `infra/**` | Domain stays free of infrastructure. | | ||
|
|
||
| ### Cross-domain isolation — `src/cross-domain.arch.spec.ts` | ||
|
|
||
| | Rule | Guards | | ||
| | ----------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `packages/<domain>/src` ↛ `packages/<other-domain>/src` (one rule per domain) | Domains collaborate only through `@packmind/types` ports wired by the HexaRegistry — never by importing each other's source. Complements the Nx `env:*` tag boundaries. (`standard-port-adapter-cross-domain-integration`) | | ||
|
|
||
| Domains are **discovered**, not hardcoded: every package under `packages/` that | ||
| owns a `src/domain/` layer is a domain (today: `accounts`, `spaces`, `standards`, | ||
| `recipes`, `skills`, `git`, `deployments`, `coding-agent`, `llm`). Shared/leaf | ||
| packages (`types`, `logger`, `node-utils`, `test-utils`, `migrations`, `editions`, | ||
| `frontend`, `assets`, `linter-*`) have no `domain/` layer, so importing them stays | ||
| legal for everyone. The only hardcoded list is `EXCLUDED_PACKAGES` in | ||
| `src/architecture.ts`, which contains just `ui` (the Chakra frontend library — not | ||
| a backend hexagon). A new domain package is therefore covered automatically. | ||
|
|
||
| ### Shared-package purity & reverse dependencies — `src/boundaries.arch.spec.ts` | ||
|
|
||
| Between packages the graph is layered too: | ||
| `apps/api → domain packages → @packmind/types (+ node-utils, logger)`. | ||
|
|
||
| | Rule | Guards | | ||
| | ------------------------------------ | --------------------------------------------------------------------------------------------------------------- | | ||
| | `@packmind/types` ↛ any domain | The contract package is a leaf — depending on a domain is a cycle and breaks "ports live in `@packmind/types`". | | ||
| | `node-utils` / `logger` ↛ any domain | Base packages everything builds on must not depend back on a domain. | | ||
| | any domain ↛ `apps/api` | Reverse dependency: the API consumes domains, never the other way round. | | ||
|
|
||
| ## Known violations (tech debt) | ||
|
|
||
| At their introduction, **16 of 23 rules passed** and **7 failed** — the | ||
| violations listed below. Failures are real and left visible on purpose: each is either | ||
| fixed by refactoring the offending import or accepted as tracked debt. (As these | ||
| imports are refactored, the corresponding entries below stop failing.) | ||
|
|
||
| **Core layering** | ||
|
|
||
| - `infra/repositories ↛ application` — `coding-agent` `PackmindDeployer` imports | ||
| `CommandsIndexService` and `StandardsIndexService`. | ||
| - `useCases ↛ infra/repositories` — three `git` use cases import | ||
| `GithubTokenResolverFactory` (`addGitProvider`, `updateGitProvider`, | ||
| `shared/validateProviderCredentials`). | ||
| - `adapter ↛ infra/repositories` — `git` `GitAdapter` → `GithubTokenResolverFactory`; | ||
| `llm` `LlmAdapter` → `AIProviderRepository`. | ||
| - `services ↛ infra/repositories` — `recipes` `RecipeService` and | ||
| `RecipeVersionService` import their concrete repositories directly. | ||
|
|
||
| **Domain purity** | ||
|
|
||
| - `domain ↛ application` — `domain/jobs/*DelayedJobs` interfaces import concrete | ||
| `application/jobs/*DelayedJob` classes in `deployments`, `git`, `recipes` (×2) | ||
| and `standards`. | ||
| - `domain ↛ infra` — `coding-agent` `ICodingAgentDeployer` imports | ||
| `DefaultSkillsDeployer`. | ||
|
|
||
| **Cross-domain** | ||
|
|
||
| - `deployments` imports `coding-agent`, `recipes`, `skills`, `spaces` and | ||
| `standards` directly (use cases / `DefaultSkillsMetadataEnricher` → | ||
| `coding-agent`; `PackageRepository` → the four others). The other seven domains | ||
| are cleanly isolated. | ||
|
|
||
| Standards referenced above live under | ||
| `packages/.claude/rules/packmind/` and `apps/api/.claude/rules/packmind/`. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| const { compilerOptions } = require('../../tsconfig.base.effective.json'); | ||
|
|
||
| const { | ||
| pathsToModuleNameMapper, | ||
| swcTransform, | ||
| standardTransformIgnorePatterns, | ||
| standardModuleFileExtensions, | ||
| } = require('../../jest-utils.ts'); | ||
|
|
||
| // Named `jest.arch.config.ts` (not `jest.config.ts`) on purpose: it keeps the | ||
| // @nx/jest plugin from inferring a `test` target for this project, so the | ||
| // architecture suite stays OFF `nx run-many -t test` and is only ever run via | ||
| // the dedicated `arch` target (`npm run test:arch`). | ||
| module.exports = { | ||
| displayName: 'architecture-tests', | ||
| preset: '../../jest.preset.ts', | ||
| rootDir: '.', | ||
| testEnvironment: 'node', | ||
| testMatch: ['<rootDir>/src/**/*.arch.spec.ts'], | ||
| transform: swcTransform, | ||
| transformIgnorePatterns: standardTransformIgnorePatterns, | ||
| moduleFileExtensions: standardModuleFileExtensions, | ||
| coverageDirectory: '../../coverage/packages/architecture-tests', | ||
| moduleNameMapper: pathsToModuleNameMapper( | ||
| compilerOptions.paths, | ||
| '<rootDir>/../../', | ||
| ), | ||
| }; |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| { | ||
| "name": "architecture-tests", | ||
| "$schema": "../../node_modules/nx/schemas/project-schema.json", | ||
| "sourceRoot": "packages/architecture-tests/src", | ||
| "projectType": "library", | ||
| "tags": ["env:node"], | ||
| "targets": { | ||
| "arch": { | ||
| "executor": "nx:run-commands", | ||
| "cache": true, | ||
| "inputs": [ | ||
| "{workspaceRoot}/packages/*/src/**/*.ts", | ||
| "{workspaceRoot}/apps/api/src/**/*.ts", | ||
| "{workspaceRoot}/packages/architecture-tests/src/**/*.ts", | ||
| "{workspaceRoot}/packages/architecture-tests/jest.arch.config.ts", | ||
| "{workspaceRoot}/scripts/build-arch-tsconfig.mjs", | ||
| "{workspaceRoot}/scripts/select-tsconfig.mjs", | ||
| "{workspaceRoot}/tsconfig.base.json", | ||
| "{workspaceRoot}/tsconfig.paths.oss.json", | ||
| "{workspaceRoot}/tsconfig.paths.proprietary.json", | ||
| "{workspaceRoot}/jest.preset.ts", | ||
| "{workspaceRoot}/jest-utils.ts", | ||
| { "env": "PACKMIND_EDITION" }, | ||
| { "externalDependencies": ["archunit", "jest"] } | ||
| ], | ||
| "outputs": [], | ||
| "options": { | ||
| "parallel": false, | ||
| "commands": [ | ||
| "node scripts/build-arch-tsconfig.mjs", | ||
| "NODE_OPTIONS='--max-old-space-size=8192' jest --config packages/architecture-tests/jest.arch.config.ts --runInBand" | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.