docs: layered-architecture guideline + import-linter pilot#380
Conversation
adriannoes
left a comment
adriannoes
left a comment
There was a problem hiding this comment.
Summary
Solid docs and import-linter pilot: the guideline fills the intra-package gap cleanly, the physical spine is honest about not matching hexagonal role names 1:1, and the next framework-free ratchet is recorded disabled instead of over-claimed. Local verification was checkout plus lint-imports on the PR head (KEPT) and on a merge with current dev (BROKEN, matching CI).
What worked well
- Clear separation from
docs/dependencies.md/ TID251 (inter-package) versus this intra-package model - Lock holds
richat 14.0.0 so the import-linter refresh does not drag an unrelated bump - Same author already opened #381 to clear the back-edge this gate detects
Required before merge
F1. Merging this branch into current dev breaks the new lint-imports gate. The PR head alone keeps the layers contract, but GitHub CI runs the merge commit, and that tree still has pipefy_mcp.core.tool_middleware importing pipefy_mcp.tools.tool_error_envelope from #378.
Please land #381 first (or an equivalent move of the envelope into core), rebase this branch, and confirm cd packages/mcp && uv run lint-imports is green on the merge with dev. The Testing section numbers look correct for the head alone; they do not describe merge CI until that rebase lands.
Verification
cd packages/mcp && uv run lint-imports(After rebase onto #381 / updated dev, the merge tree should report 1 kept, 0 broken.)
State the layer model within a package (domain, adapter, composition root), the inward-only dependency rule, type ownership at boundaries, the port and inversion seam, the per-app composition root, and a decision guide for alternative constructors. Fills the intra-package gap left by docs/dependencies.md (inter-package direction) and AGENTS.md (boundary parsing, per-app runtime), and reconciles with those existing rules rather than replacing them.
Add import-linter (root dev group) and a layers contract in packages/mcp/pyproject.toml that locks the inward-only spine server > tools > core > auth > settings, run in CI beside the ruff banned-import steps. The stricter framework-free-domain contract is recorded disabled until the scattered domain helpers get a dedicated module. core/ gains an __init__.py so pipefy_mcp.core is a discoverable package like its auth/ and tools/ siblings; the layers contract needs the intermediate node. The lock holds rich at 14.0.0 so adding import-linter does not drag in an unrelated rich bump.
Add docs/architecture.md to the AGENTS.md documentation map and docs/README.md index, a pointer from the AGENTS.md composition section, and a link from packages/mcp/AGENTS.md.
…ple citations The guideline read as mcp-specific: rules were stated once and illustrated only with pipefy_mcp, and phrases like "the two ports of this package" sat inside universal-rule sections. Add an explicit repo-wide scope paragraph and relabel the pipefy_mcp specifics as the first enforced instance, not the definition. Also drop citations to the numbered coding principles, which live only in a user-global file and are unresolvable for anyone cloning the repo; the principle content is now stated inline.
PR #381 relocated tool_error_envelope.py from tools/ to core/. Refresh the two spots that still described it under tools/: the domain mapping in docs/architecture.md and the disabled-ratchet comment in pyproject.toml. Also name core/tool_middleware.py alongside fastmcp_tool_lifecycle.py as adapter code that currently lives under core/.
808b36f to
b754d85
Compare
adriannoes
left a comment
adriannoes
left a comment
There was a problem hiding this comment.
F1 is cleared: #381 is on dev, this branch is rebased, and lint-imports keeps the contract on the merge tree.
Thanks for addressing the feedback so cleanly, Gabriel :)
2 participants
Summary
Encode project-level architecture rules as a written guideline and start enforcing intra-package layering.
The repo already documents and enforces the direction between packages (docs/dependencies.md + ruff TID251), and states boundary parsing / self-guaranteeing types / the per-app runtime (AGENTS.md, coding principles). What was missing is the layer model within a package: which module is domain, which is an adapter, which is the composition root, and what each may import. This PR fills that gap and reinforces the existing rules rather than replacing them.
What changed
docs/architecture.md: layer model (hexagonal, thin core), inward-only dependency rule, type ownership at boundaries, ports and dependency inversion, per-app composition root, an alternative-constructor decision table, testing at boundaries, enforcement, and a reconciliation note. Cites real precedents (StartupIdentity.from_configured_credential,McpRuntime.for_profile,require_request_bearer,JwtTokenVerifier,build_pipefy_mcp_server).packages/mcp: alayerscontract inpackages/mcp/pyproject.tomllocking the inward-only spineserver > tools > core > auth > settings, run in CI (cd packages/mcp && uv run lint-imports) beside the existing banned-import steps. The stricter framework-free-domain contract is recorded disabled until the domain has a dedicated module.AGENTS.md,docs/README.md, andpackages/mcp/AGENTS.md.Notes
core/__init__.pyadded sopipefy_mcp.coreis a discoverable package like itsauth//tools/siblings (the layers contract needs the intermediate node). One-line docstring, behavior-neutral.richat 14.0.0: adding import-linter forced a lock refresh that would otherwise have bumpedrich14 to 15 (constraint isrich>=13, the lock was stale), which breaks the CLI help golden test. Lock diff is import-linter + grimp only.pipefy_mcp; other packages keep their TID251 inter-package guards. Extending intra-package contracts to them is a later step.Testing
cd packages/mcp && uv run lint-imports: 1 kept, 0 broken (55 files, 155 deps).uv run ruff checkandruff format --checkonpackages/mcp/src: clean.uv run python -m pytest -m "not integration": 3261 passed, 5 skipped.