Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/test-api.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ jobs:
- name: Ruff format check
run: uv run --no-sync ruff format --check .

- name: Tach check
run: uv run --no-sync tach check

- name: Pytest
run: uv run --no-sync pytest
env:
Expand Down
7 changes: 7 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ repos:
# pass_filenames: false
# stages: [pre-push]

- id: tach
name: tach
entry: bash -c 'cd api && uv run --group lint tach check'
language: system
files: ^api/
pass_filenames: false

- id: frontend-eslint
name: frontend-eslint
entry: bash -c '. scripts/ensure-node.sh && cd frontend && pnpm exec eslint --fix --cache --cache-location node_modules/.cache/eslint/.eslintcache "${@#frontend/}"' --
Expand Down
9 changes: 4 additions & 5 deletions api/docs/adr/000-vertical-slice-architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,16 @@ Chosen option: a combination of **Vertical Slice Architecture** and **Ports and

Slices give change-locality, a feature change touches one directory, and a reviewer can hold a slice in their head. Ports are applied only at the I/O boundaries (the external DAMNIT databases, MyMdC, auth, etc...), where swappability is useful (e.g. local dev vs. production, potential use at other facilities).

The dependency-direction rules that layering-style architectures enforce through folder structure can instead be enforced by a linter (e.g. import linter).
The dependency-direction rules that layering-style architectures enforce through folder structure are instead enforced by an import linter (tach).

Full layering, Clean/Onion, and DDD, were rejected as they add a lot of boilerplate/abstraction/overhead to the codebase which is (at least currently) not needed, as the API server is a relatively thin, read-mostly viewer over externally-owned data, so patterns like aggregates, domain events, and use-case classes solve problems this service doesn't have.

### Consequences

- Good: a feature change touches one directory; PRs map to slices.
- Good: new domains get a package with a standard internal shape (`models`, `services`, `routers`/`gql`, `dependencies`), so structure decisions don't recur per feature.
- Good: once import linting lands, contract changes (a new allowed edge) become deliberate, reviewed edits to the linter config rather than drive-by imports.
- Bad: the layout alone guarantees nothing, developers have to ensure that they follow the architecture (although an import linter can be added to enforce the rules).
- Good: contract changes (a new allowed edge) are deliberate, reviewed edits to the tach config rather than drive-by imports.
- Bad: the layout alone guarantees nothing. The rules hold because tach enforces them, not because of the folder structure.

## Details

Expand Down Expand Up @@ -95,7 +95,7 @@ damnit_api/

### Naming Rules

- No `_underscore` package names: the prefix tracks no real boundary - a package is internal because nothing outside imports it, which import linting can enforce. (This is why `_db/` and `_mymdc/` become `appdb/` and `mymdc/`.)
- No `_underscore` package names: the prefix tracks no real boundary - a package is internal because nothing outside imports it, which tach enforces. (This is why `_db/` and `_mymdc/` become `appdb/` and `mymdc/`.)
- No generic junk-drawer modules (`shared/`, `utils.py`): code either belongs to a feature slice, to `core/` (framework-free, shared), or to infrastructure.

### Dependency Direction
Expand All @@ -110,5 +110,4 @@ damnit_api/

### Follow-up / TODOs

- Import linting
- `appdb` naming
2 changes: 1 addition & 1 deletion api/docs/adr/002-no-global-mutable-state.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ Chosen option: a single frozen `AppState` dataclass built once in the lifespan.
3. There is exactly one composition/setup root: the app entrypoint and its lifespan (target shape: `create_app(settings)`, see the ADR-000 layout).
- This is the only place that reads settings to select implementations.
- Handlers, resolvers, and services receive dependencies via DI or plain parameters, they must **never** import them.
<!-- TODO: add to import linting -->
- tach enforces this: nothing may import the composition root (`state.py`, `main.py`), so a handler cannot reach `AppState` by import.
4. Caches must be treated as state.
- Any cache must be owned by an object that is itself created by a factory and reachable from `AppState`.
- Module-level and class-level cache decorators on application code are banned.
10 changes: 5 additions & 5 deletions api/docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ flowchart LR
root --> slices --> infra --> core
```

The key rules are:
Rules, enforced by [tach](https://github.com/gauge-sh/tach) (`api/tach.toml`, wired into CI and pre-commit):

1. **Downward only:** Slices import `core` and infrastructure
- Slices never import the composition root or another slice's internals.
Expand All @@ -60,10 +60,10 @@ The key rules are:
- This means that services should not apply authorisation rules themselves.
5. **No `if settings.is_local:` outside the composition root:** Local mode is selected by composition, not conditionals throughout the codebase (see [ADR-008](adr/008-local-mode-composition.md)).

Note that these are currently only enforced by convention/review. Import linter/archetecture check tool is planned to be added.

!!! warning "Current issues"

The current code still violates some of these rules. Each violation is declared as a `DEBT(ADR-xxx)` edge in `api/tach.toml`, so `tach check` fails on any new one:

- `shared/gql.py`'s import-everything role
- Function-body imports working around circular imports
- Imports 'across' many modules and their files
- `graphql/` resolvers importing `runs` and `metadata` directly
- `auth` and `metadata` importing `runs` for local-mode proposal lookup
1 change: 1 addition & 0 deletions api/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ test = [
lint = [
"pyright>=1.1.406",
"ruff>=0.7",
"tach>=0.29",
"ty>=0.0.42",
]
docs = [
Expand Down
167 changes: 167 additions & 0 deletions api/tach.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
# Import-boundary baseline (ADR-000).
#
# This encodes the *current* dependency graph, derived from real imports via
# `tach sync`, not the target architecture in docs/architecture.md. Edges that
# the architecture forbids are marked DEBT(ADR-xxx) with a one-line reason;
# un-annotated edges are legitimate under the current rules (composition-root
# imports, core/infrastructure imports, or the one explicit auth->proposals
# exception). Don't add new DEBT edges; fixing an existing one means deleting
# its line here in the same commit.

source_roots = ["src"]

[[modules]]
path = "damnit_api"
# legitimate: the package __init__ configures logging on import.
depends_on = ["damnit_api._logging"]

[[modules]]
path = "damnit_api.auth"
depends_on = [
"damnit_api",
"damnit_api.shared",
"damnit_api._db",
"damnit_api._mymdc",
# legitimate: ADR-000's explicit "auth -> proposals" exception (metadata is
# proposals under its current name); membership needs proposal metadata,
# never the reverse.
"damnit_api.metadata",
# DEBT(ADR-000): auth/routers.py's noauth_userinfo() imports
# runs.dependencies.Repositories for local-mode proposal lookup. Slice ->
# slice import with no declared exception.
"damnit_api.runs",
]

[[modules]]
path = "damnit_api.contextfile"
# legitimate: contextfile resolves proposal metadata/path to serve context
# files, the future proposals/locator.py role (ADR-004).
depends_on = ["damnit_api.metadata"]

[[modules]]
path = "damnit_api.metadata"
depends_on = [
"damnit_api",
"damnit_api.shared",
"damnit_api._db",
"damnit_api._mymdc",
# DEBT(ADR-000): metadata/services.py's _local_proposal_number() imports
# runs.repository / runs.sqlite.repository (function-body) for local-mode
# lookup. Slice -> slice import with no declared exception.
"damnit_api.runs",
]

[[modules]]
path = "damnit_api.runs"
# legitimate: core (shared) plus the to-be-removed utils junk drawer.
depends_on = ["damnit_api", "damnit_api.shared", "damnit_api.utils"]

[[modules]]
path = "damnit_api.graphql"
depends_on = [
"damnit_api",
"damnit_api.shared",
# DEBT(ADR-007): graphql is transport-only, but graphql/queries.py,
# subscriptions.py, directives.py and publisher.py import runs.types /
# runs.repository directly. Resolves once resolvers move to runs/gql.py.
"damnit_api.runs",
# DEBT(ADR-007): graphql/queries.py imports metadata.services'
# _get_proposal_meta / _update_proposal_meta - cross-slice private names
# (also ADR-000). Resolves once resolvers move to a slice gql module and
# metadata gets a public interface.
"damnit_api.metadata",
]

[[modules]]
path = "damnit_api.shared"
depends_on = [
"damnit_api",
# DEBT(ADR-007): shared/gql.py assembles the whole schema and Context -
# composition-root-shaped, importing from nearly every package - but lives
# under shared/ instead of graphql/schema.py. Most edges below trace to it;
# once it moves to graphql/schema.py they become legitimate composition-root
# imports and this block shrinks to the two exceptions noted below.
"damnit_api._db",
"damnit_api.runs",
"damnit_api.graphql",
"damnit_api.metadata",
# DEBT(ADR-007) + shared/permissions.py imports auth.policy's
# require_proposal_member, the ADR-011 transport adapter over the membership
# policy - a core-shaped module importing a slice.
"damnit_api.auth",
# DEBT(ADR-007) + shared/settings.py imports _mymdc.settings models as part
# of settings assembly.
"damnit_api._mymdc",
]

[[modules]]
path = "damnit_api._db"
# clean: infrastructure imports nothing app-specific.
depends_on = []

[[modules]]
path = "damnit_api._mymdc"
# legitimate: infrastructure -> core.
depends_on = ["damnit_api", "damnit_api.shared"]

[[modules]]
path = "damnit_api._logging"
depends_on = ["damnit_api.shared"]

[[modules]]
path = "damnit_api.utils"
depends_on = ["damnit_api.shared"]

[[modules]]
path = "damnit_api.state"
# composition root: may import everything (architecture.md rule 2).
depends_on = ["damnit_api.runs", "damnit_api._mymdc"]

[[modules]]
path = "damnit_api.main"
# composition root: may import everything (architecture.md rule 2).
depends_on = [
"damnit_api",
"damnit_api.shared",
"damnit_api.runs",
"damnit_api.graphql",
"damnit_api.metadata",
"damnit_api.contextfile",
"damnit_api.auth",
"damnit_api._mymdc",
"damnit_api._logging",
"damnit_api.state",
]

# Public-interface enforcement: modules with no known cross-slice `_private`
# imports get an interface restricting them to non-underscore names, so a new
# private cross-slice import fails loudly. `metadata` is deliberately left off -
# its genuine `_get_proposal_meta` / `_update_proposal_meta` /
# `_get_proposal_meta_many` / `_local_proposal_meta` / `_local_proposal_number`
# cross-slice imports (see the DEBT comments above) would fail this check;
# bending the tool to pass would hide real debt, so it stays an uncovered gap
# (ADR-000).

[[interfaces]]
expose = ["^[^_][^.]*(\\.[^_][^.]*)*$"]
from = ["damnit_api"]

[[interfaces]]
expose = ["^[^_][^.]*(\\.[^_][^.]*)*$"]
from = ["damnit_api.auth"]

[[interfaces]]
expose = ["^[^_][^.]*(\\.[^_][^.]*)*$"]
from = ["damnit_api.contextfile"]

[[interfaces]]
expose = ["^[^_][^.]*(\\.[^_][^.]*)*$"]
from = ["damnit_api.shared"]

[[interfaces]]
expose = ["^[^_][^.]*(\\.[^_][^.]*)*$"]
from = ["damnit_api._db"]

[[interfaces]]
expose = ["^[^_][^.]*(\\.[^_][^.]*)*$"]
from = ["damnit_api._mymdc"]
Loading
Loading