From dcdba990c56af753652c40d90bdc3fbce32d74a2 Mon Sep 17 00:00:00 2001 From: n Date: Wed, 5 Aug 2026 23:18:52 -0500 Subject: [PATCH] Split the reviewer conventions across the lenses that own them Every lens was handed the same list of Crypter conventions while also being told to stay inside its lens, so the security lens was primed to report a missing Async suffix and forbidden from reporting it in the same breath. Which way a reviewer resolved that was left to chance. The conventions are now split by what goes wrong when they are broken, and each half lives in the lens brief that can judge the consequence: monads, sync IO, constructors, enums and missing migrations under correctness; history-narrating comments and Async naming under maintainability; validated primitives under security. Testability claims none, rather than being given one to justify the symmetry. That moves them out of the shared agent definition and into the prompt, where the skill already said the lens belongs. The briefs must now be passed in full for the conventions to be reviewed at all. Co-Authored-By: Claude Opus 5 --- .claude/agents/reviewer.md | 15 ++----- .../crypter-devcontainer-examine/SKILL.md | 41 +++++++++++++++---- 2 files changed, 37 insertions(+), 19 deletions(-) diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index ada4f1ce..d2bdf974 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -47,17 +47,10 @@ nits, and the nits make the real ones harder to see. ## Crypter's conventions are in scope -A change that ignores them is a legitimate finding for any lens: - -- Nulls or exceptions where `Maybe` or `Either` from `Crypter.Common/Monads` - belongs. -- Raw strings where a validated type from `Crypter.Common/Primitives` exists. -- Sync IO on a database, file, or network path; a missing `Async` suffix. -- Object initializers where a constructor belongs; magic strings where an enum belongs. -- An entity change under `Crypter.DataAccess/Entities` with no migration in - `Crypter.DataAccess/Migrations` — and whether it needs a companion script in - `Crypter.DataAccess/Scripts`. -- Comments narrating history rather than explaining the code as it stands. +A change that ignores the conventions in `CLAUDE.md` and the Coding Standard is a legitimate +finding, and your lens names the ones that are yours. They are split across the lenses by what +goes wrong when they are broken, so a convention yours does not name is another lens's — leave +it, the same as anything else outside your brief. ## Report diff --git a/.claude/skills/crypter-devcontainer-examine/SKILL.md b/.claude/skills/crypter-devcontainer-examine/SKILL.md index df2dbe59..c22924ea 100644 --- a/.claude/skills/crypter-devcontainer-examine/SKILL.md +++ b/.claude/skills/crypter-devcontainer-examine/SKILL.md @@ -62,16 +62,41 @@ Given a plan path, invoke `conformance-auditor` with it, the workspace, the base ## 3. Code review Invoke `reviewer` once per lens, in parallel — they do not interact. Each gets the workspace, the -base ref, and `/runs/{run-id}/findings/{lens}.md`. +base ref, `/runs/{run-id}/findings/{lens}.md`, and its brief below, in full. -| Lens | Brief | -|---|---| -| correctness | Bugs, boundary conditions, error paths, and what happens when inputs are hostile or absent. | -| maintainability | Readability, scope creep, and the conventions in `CLAUDE.md` and the Coding Standard. | -| testability | What the tests pin down, what they leave unverified, and whether the change can be tested at all. | -| security | Crypto boundaries, input validation, authentication and authorisation paths, key handling, transfer integrity. | +A brief is the whole of what its lens covers, conventions included. The conventions are split by +what goes wrong when they are broken rather than kept as one list, so that a lens is told the +ones it can judge the consequences of and left ignorant of the rest. + +**correctness** — Bugs, boundary conditions, error paths, and what happens when inputs are +hostile or absent. Including: + +- Nulls or exceptions where `Maybe` or `Either` from `Crypter.Common/Monads` + belongs, and the crash or swallowed failure that follows. +- Sync IO on a database, file, or network path. +- Object initializers where a constructor belongs, leaving an object usable before it is whole. +- Magic strings where an enum belongs. +- An entity change under `Crypter.DataAccess/Entities` with no migration in + `Crypter.DataAccess/Migrations`, and whether it needs a companion script in + `Crypter.DataAccess/Scripts`. + +**maintainability** — Readability, scope creep, and the conventions in `CLAUDE.md` and the Coding +Standard that no other lens claims. Including: + +- Comments narrating history rather than explaining the code as it stands. +- A missing `Async` suffix on an async method. The naming is yours; sync IO on a path that should + be async belongs to correctness. + +**testability** — What the tests pin down, what they leave unverified, and whether the change can +be tested at all. + +**security** — Crypto boundaries, input validation, authentication and authorisation paths, key +handling, transfer integrity. Including: + +- Raw strings where a validated type from `Crypter.Common/Primitives` exists, and the unchecked + value that reaches past a boundary as a result. -Adding a lens means adding a row here. The `reviewer` definition stays as it is; the lens comes +Adding a lens means adding a brief here. The `reviewer` definition stays as it is; the lens comes from the prompt. Run the phases in parallel with each other too. The auditor and the reviewers read the same diff