docs(conventions): document the modularity checks across RAG, prompts, and lifecycle diagrams

Make the docs and role prompts match the shipped modularity enforcement. The standards doc gains a Modularity section (cohesion / thin routes / thin components / god class, scan-derived + language-aware); the developer prompt tells agents to write modular code (thin routes that delegate, one concern per file, components that delegate to hooks) and that block-level findings refuse i_am_done; QA + PR-reviewer prompts note the modularity findings in evidence / the pr_pass block. Also fixes the two lifecycle diagrams (usage.md, roboco/models/README.md) that omitted the awaiting_pr_review gate.
This commit is contained in:
Renn F
2026-06-22 14:38:51 +02:00
parent c54bca4c21
commit 3baac6dcd8
6 changed files with 66 additions and 21 deletions
+11
View File
@@ -106,6 +106,17 @@ The gateway enforces some of these; the rest are convention but failing one of t
If any item fails, do not retry `i_am_done`; fix the missing piece first.
## Write modular code — the conventions gate enforces it
Beyond placement and hygiene, the Architectural Conventions Standard now enforces MODULARIZATION via a "modularity" AST check family that inspects a definition's body and a file's composition. Write to it from the start — a block-level modularity finding refuses `i_am_done` (and the PR reviewer's `pr_pass`) with the offending `file:line` + a fix hint, and surfaces in QA's `claim_review` evidence as `convention_findings`. The checks are language-aware: a Python/API project carries `thin_routes`; a TypeScript/React project carries `thin_components`; `modular_cohesion` and `god_class` apply to both.
- **One architectural concern per file (`modular_cohesion`).** A file must own a single concern. Do not define a Pydantic model inside a router, or a schema inside a component — split each concern into its own module (`models/`, `schemas/`, the hook, …).
- **Keep route handlers thin (`thin_routes`, Python/API).** A route delegates data access and business logic to a service. It must NOT run its own database access in the route body — no `session.execute`/`query`/`commit`/`add`, no `select()`/`insert()`/`update()`/`delete()`. Move that into the service the route calls.
- **Keep components presentational (`thin_components`, TypeScript/React).** Data fetching (`fetch`/`axios`) and logic belong in a hook, not the component body. The component renders; the hook fetches.
- **No god classes (`god_class`).** A class past the method-count threshold is doing too much — decompose it along its responsibilities.
If a finding is a genuine false positive, clear it by committing a `waiver` in `.roboco/conventions.yml` in your branch — accountable and reviewed in the PR. Do not silence it any other way.
## Channels
**Before any `say(channel=...)` call if you're unsure of the slug**, call `channels()` to list the channels you have read/write access to. Inventing a slug returns `Channel not found`. The returned `writable` list is the canonical set; pick from there.
+1
View File
@@ -44,6 +44,7 @@ The PR is from an outside contributor: its code is **untrusted**. Until a human
- ❌ A trickle of vague comments. Post ONE complete review; each finding names file + line + expected vs actual.
- ❌ Approving without reading the full diff.
- ❌ Being lax on the architectural standard. Be mega-strict: on an in-path gate review, a `block`-level convention violation (a definition in the wrong module per `.roboco/conventions.yml`, a helper/model in a router, a lint/type suppression) is an automatic `pr_fail` — the gate already refuses `pr_pass`, and an introduced or expanded `waiver` must be justified in the diff or rejected. Hold placement and house-style to the same bar as correctness.
- ❌ Letting a non-modular assembled change through. The standard also enforces **modularity** (`modular_cohesion`, `thin_routes`, `thin_components`, `god_class`): a file must own one architectural concern (no model in a router, no schema in a component), a route handler must delegate to a service rather than run its own DB access in the route body, a React component must stay presentational with data fetching in a hook, and a class past the method-count threshold must be decomposed. A `block`-level modularity finding refuses `pr_pass` exactly the way it refuses the developer's `i_am_done` — these surface in QA's `claim_review` evidence as `convention_findings`, carry the offending `file:line` + a fix hint, and clear only via a `waiver` committed in the branch.
## When the gateway returns an error
+1 -1
View File
@@ -79,7 +79,7 @@ The gateway requires `learning` before `pass`/`fail`. Your `notes` argument carr
6.`note(scope='learning', task_id=...)` written.
7. ✅ For `pass`: `notes` >= 80 chars, names the criteria you verified and the artifact behind each.
8. ✅ For `fail`: each entry in `issues` is concrete and actionable — criterion + file + line + expected/actual. "Doesn't work" is not an issue.
9. ✅ Read `convention_findings` in your `claim_review` evidence — it lists architectural-standard violations on the diff (misplaced definitions, lint suppressions). Flag any block-level finding in your `issues`; a `could_not_run` entry means the validator failed and the placement is unverified, so don't pass on a clean-looking diff.
9. ✅ Read `convention_findings` in your `claim_review` evidence — it lists architectural-standard violations on the diff (misplaced definitions, lint suppressions). Modularity findings (`modular_cohesion` — a file mixing more than one architectural concern, e.g. a model defined in a router; `thin_routes` — a Python route handler running its own DB access instead of delegating to a service; `thin_components` — a React component fetching data in its body instead of in a hook; `god_class` — a class past the method-count threshold) appear here too, alongside the placement and hygiene findings. Flag any block-level finding in your `issues`; a `could_not_run` entry means the validator failed and the placement is unverified, so don't pass on a clean-looking diff.
## Channels
+14 -1
View File
@@ -1,6 +1,6 @@
# Architectural Conventions Standard
A per-project, repo-canonical standard for *where code lives* and basic house-style hygiene — the layer above the `make`-style gates (which check syntax, types, and tests, not placement). It exists so an agent cannot land a model defined inside a router, a helper in a route file, or a lint suppression, even when the code compiles and the tests pass.
A per-project, repo-canonical standard for *where code lives*, how a definition is *built*, and basic house-style hygiene — the layer above the `make`-style gates (which check syntax, types, and tests, not placement or structure). It exists so an agent cannot land a model defined inside a router, a route handler that runs its own database access, a helper in a route file, or a lint suppression, even when the code compiles and the tests pass. It enforces the separation of concerns a senior would demand in review, not just linting.
The standard is gated by `ROBOCO_CONVENTIONS_ENABLED` (default off) and is fully inert when off.
@@ -54,6 +54,19 @@ python -m roboco.conventions check --root <repo> --files <a> <b> ...
It favours precision over recall — it abstains when it cannot classify a definition, so a `block` gate can never strand a task on a guess — and it fails loud: a validator that cannot run exits non-zero so the gate blocks rather than silently passing.
## Modularity
Beyond placement and hygiene, the standard enforces modularization with a **modularity** check family. Where placement asks *which module a definition belongs in*, modularity inspects a definition's **body** and a file's **composition** — the structural questions a senior asks in code review:
- **`modular_cohesion`** (any stack) — a file must own one architectural concern. A file that mixes them (a Pydantic model defined in a router, a schema defined in a component) is a monolith to split.
- **`thin_routes`** (Python / API) — a route handler must delegate to a service. It may not run its own database access (no `session.execute` / `query` / `commit` / `add`, no `select()` / `insert()` / `update()` / `delete()`) in the route body.
- **`thin_components`** (TypeScript / React) — a component must stay presentational. Data fetching (`fetch` / `axios`) belongs in a hook, not in the component body.
- **`god_class`** (any stack) — a class past a method-count threshold is doing too much; decompose it to keep a single responsibility.
Rules are scan-derived and language-aware: hygiene seeds universally, placement only for modules that actually exist in the repo, and modularity per stack — a Python project gets `thin_routes`, a TypeScript project gets `thin_components`, and `modular_cohesion` plus `god_class` apply to both. A frontend project therefore carries `no_models_in_components` and `thin_components`, never a backend `no_models_in_routers`.
Modularity findings flow through the same enforcement as the rest of the standard: a `block`-level finding refuses the developer's `i_am_done` and the reviewer's `pr_pass` with the offending `file:line` and a fix hint, and surfaces in QA's `claim_review` evidence as `convention_findings`. A false positive is cleared the same way — a waiver committed in the branch.
## Where it is enforced
Enforcement is deterministic and reaches the work two ways: an ambient "Architectural Standard" block injected into an agent's context at spawn, and an auto-attached `## Constraints` section on every project task.
+33 -19
View File
@@ -52,25 +52,39 @@ From `roboco/enforcement/task_lifecycle.py`:
backlog ────────► pending ────────► claimed ────────► in_progress
│ │ │
▼ ▼ ├──► blocked ──► in_progress
cancelled cancelled ──► paused ───► in_progress
├──► verifying
├──► awaiting_pm_review ──► completed
│ │
│ ▼
│ awaiting_ceo_approval ──► completed
│ │
│ ▼
│ needs_revision
awaiting_qa
┌───────────┴───────────┐
▼ ▼
awaiting_documentation needs_revision
awaiting_pm_review
cancelled cancelled ──► paused ───► in_progress
Two flows leave in_progress depending on the task:
Leaf developer task (self-verify → QA → docs → PM merge):
in_progress ──► verifying ──► awaiting_qa
┌───────────┴───────────┐
▼ ▼
awaiting_documentation needs_revision
awaiting_pm_review ──► completed
awaiting_ceo_approval ──► completed
needs_revision
Assembled parent task the PM submits (submit_up opens the cell→root PR,
submit_root opens the root→master PR; reviewed at the in-path PR gate):
in_progress ──► awaiting_pr_review
┌──────────┴──────────┐
│ pr_pass │ pr_fail
▼ ▼
awaiting_pm_review needs_revision
completed
```
### Terminal States
+6
View File
@@ -129,8 +129,14 @@ Enum values: `task_type` ∈ {`code`, `documentation`, `research`, `planning`, `
pending → claimed → in_progress → verifying → awaiting_qa → awaiting_documentation → awaiting_pm_review → completed
↓ ↓
blocked/paused awaiting_ceo_approval (major tasks)
in_progress → awaiting_pr_review → awaiting_pm_review → completed (in-path PR-review gate for assembled PRs)
needs_revision (pr_fail)
```
The leaf flow above is the developer path. Assembled PRs add an in-path PR-review gate: the cell PM's `submit_up` opens the cell→root PR and the Main PM's `submit_root` opens the root→master PR, each moving the task `in_progress → awaiting_pr_review`. There the PR reviewer `pr_pass`es it on to `awaiting_pm_review` or `pr_fail`s it back to `needs_revision`. Leaf developer tasks and branchless coordination roots skip the gate.
Agents automatically:
1. Pull pending work via the gateway verb `give_me_work()`
2. Claim it with `i_will_work_on(task_id)` (auto-creates the feature branch)