Close four gold criteria, and measure the two that were guesses

CONTRIBUTING.md

@@ -39,12 +39,55 @@ evidence. The short version:
   wrong line stays and is corrected below it, saying what it supersedes.
 - **A failing test comes before the fix.** A test for a claimed property is committed failing
   first, by name, and the fix commit refers to it.
+- **Review has a written standard.** What a reviewer checks, and what makes a change acceptable, is
+  the *Code review* section of `docs/03-conventions.md`. Read it before opening a pull request: it
+  is the list your change will be held against, and most of it is about evidence rather than style.
 - **Major new functionality comes with tests for it, in the same change.** This is a policy, not a
   preference: a change that adds behaviour and no test for that behaviour is incomplete, and the
   question a reviewer will ask is which test fails if the change is reverted. A fixed bug gets a
   regression test naming the defect. The full policy is the *Tests* section of
   `docs/03-conventions.md`.
 
+## Good first tasks
+
+Small, self-contained pieces of work suitable for someone who has not worked on this project
+before. Each is real: it came out of the tree rather than being invented to fill this list, and
+each names where to start.
+
+**Documentation and records**
+
+- **Check a document against the code it describes.** Pick any file in `docs/` and verify its
+  claims against the tree; anything that has drifted is a defect worth a pull request. Start with a
+  document whose slice number is old, since those have had the most time to drift.
+- **Add a worked example to `docs/mcp-server.md`.** Connecting a client to Trinity's protocol
+  server is documented as configuration; an end-to-end transcript would be more use than another
+  paragraph.
+
+**Tests**
+
+- **Widen a population test.** `test/workflows_test.exs` and `test/spdx_headers_test.exs` each take
+  their population from `git ls-files`. Other rules in `docs/03-conventions.md` are still enforced
+  by review alone and could be enforced by a test the same way. Pick one and write it.
+- **Add a test for a module below the coverage line.** `mix test --cover` prints per-module
+  coverage; anything well under the project total is a candidate. Small, useful, and it moves a
+  number the project tracks in `coverage.tsv`.
+
+**Code**
+
+- **Improve a refusal message.** The project's own rule is that a refusal says what was refused and
+  why, in the channel where it was asked. Some messages say less than that. `Trinity.Gateways.Cap`
+  and `Trinity.Permissions` are where to look.
+- **Fix a `shellcheck` style note in a workflow.** `actionlint` reports two, both in
+  `.github/workflows/`. Small, visible, and it teaches the build.
+
+**If you want something larger,** the risk register (`docs/06-risk-register.md`) lists open risks
+with their lift conditions, and R26 in particular is an unfixed intermittent test failure with
+three candidate fixes recorded and none taken. That one is not a first task, but it is a real one.
+
+Before starting anything larger than a correction, open an issue describing it, so that two people
+do not build the same thing. There is no expectation that a first contribution be large: a
+documentation fix that makes the next reader's life easier is a contribution this project wants.
+
 ## Commit messages
 
 Conventional Commits, with a scope that identifies the unit of work:

docs/03-conventions.md

@@ -109,6 +109,69 @@ These are enforced by the gate where a tool can enforce them, and by review wher
 - **Tests do not reach the network.** Provider calls are mocked against behaviours; an opt-in
   tag exists for tests that deliberately exercise a real provider.
 
+## Code review
+
+What a reviewer does, what they check, and what makes a change acceptable. Written down because a
+review standard that lives in one person's head is not a standard, and because the first question
+a reviewer of *this project* will ask is how its own changes are reviewed.
+
+### How review is conducted
+
+Every change reaches `main` through a pull request against a protected branch, with the full gate
+green. There is no direct push and no exception for a one-line fix; a change small enough not to
+need review is small enough not to need an exception.
+
+A pull request states what it changes and what it proves. The body carries the evidence: commands
+and their output, or named tests and their results. A reviewer who has to run the change to find
+out what it does is being asked to do the author's work.
+
+The reviewer is someone other than the author wherever the maintainer list allows it. Where it
+does not, the change is still gated on the full build, and the project says plainly in
+`docs/06-risk-register.md` (R22) that this is a gap rather than pretending the gate substitutes
+for a second pair of eyes.
+
+### What must be checked
+
+A reviewer is responsible for forming an opinion on each of these, not for confirming that a tool
+ran:
+
+1. **Is the change worth making?** Scope first. A change that does something nobody asked for is
+   rejected however well it is written, and anything found outside the stated scope belongs in a
+   follow-up rather than folded in quietly.
+2. **Does the evidence support the claim?** Every "verified" names its command and its exit code.
+   A summary of output is not output. Where a test is claimed to prove a property, the reviewer
+   asks which test fails if the change is reverted, and whether that test could fail at all.
+3. **Is the security argument still true?** If the change touches an effect, a permission, a
+   boundary, a token, a key or anything crossing a trust boundary, the reviewer checks it against
+   `docs/07-security-model.md` and `docs/10-assurance-case.md`, and rejects a change that makes a
+   statement in either of those documents false without correcting it.
+4. **Does it fail safe?** A new default that permits, a new path that skips the gate, an error
+   handler that repairs malformed input instead of refusing it: each of these is a defect even
+   when every test passes.
+5. **Is untrusted content still treated as untrusted?** Anything that moves content toward the
+   model, the prompt or an effect is checked for whether it preserves origin and taint.
+6. **Do the records stay honest?** Corrections append; a document is not rewritten to look as
+   though it had always been right. A commit message that describes changes the commit does not
+   make, or omits changes it does make, is sent back.
+7. **Is what is *not* done stated?** A change that leaves a gap and says so is acceptable. One that
+   leaves a gap silently is not.
+
+### What makes a change acceptable
+
+All of the following, together:
+
+- the gate is green, on the change as it will land rather than on an earlier version of it;
+- the evidence in the pull request is reproducible by the reviewer from what is written there;
+- major new functionality has tests in the same change, and a fixed defect has a regression test
+  naming it;
+- documents affected by the change are updated in the same change, including the risk register and
+  the assurance case when the change touches what they claim;
+- nothing in the change is outside its stated scope;
+- the reviewer can say what the change does without reading the diff twice.
+
+A change failing any of these is returned with the reason. "Looks fine" is not a review, and
+approving something a reviewer has not understood is worse than declining to review it.
+
 ## Rules of evidence
 
 The standard a change is held to, and the reason the proof sections of this repository can be

docs/11-security-review.md

@@ -0,0 +1,153 @@
+<!-- SPDX-FileCopyrightText: Sudo Apt Holdings LLC -->
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+# 11: Security review
+
+**Review of 2026-09-23.** Conducted by the maintainers, against the security requirements in
+`docs/07-security-model.md` and the boundary and threat model in `docs/10-assurance-case.md`.
+Supported by the tools the build runs on every commit, but the findings below are the ones a
+person found, because a review that only reports what a tool reports is a tool run with a date on
+it.
+
+This document is the record, not the certificate. It says what was examined, what was found, what
+was done about each finding, and what remains open. A review with no findings would be evidence
+that the review did not happen.
+
+## Scope
+
+**In scope.** The software in this repository: the agent core, the permission gate and effect
+membrane, the receipt chain, the protocol client and server and their authorization, the messaging
+gateways, the web interface, the build and its supply chain.
+
+**Out of scope, and why.** The host operating system, the model provider's conduct, any external
+authorization server a deployment supplies, and physical access to the machine. Each is named as an
+assumption in `docs/10-assurance-case.md`; a review cannot establish properties of things it does
+not control, and claiming otherwise would weaken the parts it can establish.
+
+## Method
+
+1. The security requirements were read as claims and each was traced to the code that implements
+   it and the test that holds it. Claims C1 to C10 in the assurance case are the result.
+2. The trust boundary was walked in both directions: what crosses inward and where it is labelled;
+   what crosses outward and what decided that it could.
+3. The threat model's five adversaries were taken in turn and the tree examined for what each would
+   try. This is where the findings below came from.
+4. Tool output was read as input to the review rather than as its conclusion: the compiler with
+   warnings as errors, the architectural boundary checker, Credo in strict mode, Sobelow, the
+   dependency and licence audits, and a CycloneDX bill validated against the specification.
+
+## Findings
+
+Ordered by what they cost, not by when they were found. Every one of these is a defect a human
+found in something that was passing its tests at the time.
+
+### F1. Release-only compile failure, shipped and tagged (resolved)
+
+**Severity: high. Class: build integrity.** A plug's `authorize:` option was an anonymous function.
+`Plug.Builder` calls `init/1` at compile time under `MIX_ENV=prod` and escapes the result, and a
+closure cannot be escaped, so the production build failed while every test passed: the gate ran in
+`:test`, where `init/1` is per request. The defect reached a tag before anything caught it.
+
+**Resolved.** Replaced with a captured public function, with a regression test that runs
+`Macro.escape/1` over `Plug.init([])`. More usefully, the *class* was closed: `mix gate` now runs
+`scripts/prod_check.sh`, which compiles `:prod`, assembles a release and evaluates it, so a
+production-only defect fails on the branch that introduces it.
+
+**What it says about the review:** the gap was not in the code but in what the build exercised. A
+test suite that never runs the configuration a user gets is not testing the product.
+
+### F2. Pairing codes from a non-cryptographic generator (resolved)
+
+**Severity: medium. Class: authentication.** Gateway pairing codes were generated with
+`Enum.random/1`, which draws from a pseudo-random source that is not cryptographically secure. A
+pairing code is what stands between an unknown account and a session.
+
+**Resolved.** `:crypto.strong_rand_bytes/1` over a 32-character alphabet, so that `256 rem 32 = 0`
+and the mapping is unbiased, with a census test over the distribution. Found by reading the code
+against the criterion for cryptographic randomness rather than by any tool.
+
+### F3. An architectural boundary that was never enforced (resolved)
+
+**Severity: medium. Class: architectural integrity.** `Trinity.MCP.Auth` declared `deps: []`,
+which reads as "this package depends on nothing and can be extracted". It could not: a nested
+sub-boundary inherits its ancestors' dependencies, so the declaration was a claim the compiler had
+never checked.
+
+**Resolved.** Declared `top_level?: true`, which makes the compiler enforce it. **The finding that
+matters is not the fix:** a declaration that looks like an enforced constraint but is not is worse
+than no declaration, because it stops people checking.
+
+### F4. Workflow tokens broader than the work (resolved)
+
+**Severity: medium. Class: supply chain.** Two build workflows declared no token permissions at
+all and ran with the repository default, which grants read on every scope; a third granted a
+package write at the workflow level, where it applied to every job it would ever have.
+
+**Resolved.** All three declare `contents: read`, with writes declared by the individual job that
+needs them, and a test now requires every workflow to declare a top-level block granting no write.
+Found by acting on a Scorecard result rather than by reading it.
+
+### F5. A bill of materials that no consumer could validate (resolved)
+
+**Severity: low. Class: supply chain.** The first CycloneDX bill this project generated was not
+valid CycloneDX. The generator emits a dependency's licence string as an SPDX identifier without
+checking that SPDX defines it, and one dependency spells `BSD 2-Clause` where SPDX has
+`BSD-2-Clause`. One field in one component of 132 invalidated the whole document.
+
+**Resolved.** Identifiers SPDX does not define are recorded as licence names, which is where
+CycloneDX puts a licence it cannot resolve, against a vendored copy of the enumeration the
+validator itself uses. Found by running the official validator instead of asserting the format.
+
+## Open
+
+Recorded here and in `docs/06-risk-register.md` rather than closed. Each names what would close it.
+
+### O1. No egress restriction by resolved address
+
+**Severity: medium. Class: server-side request forgery.** Every web fetch passes the permission
+gate, so a person decides each one and the decision binds the exact arguments. There is no egress
+allow-list and no check on the address a host resolves to, so an approved fetch to a host resolving
+to a private or link-local address is not blocked by construction. `Trinity.NetworkGuard` blocks
+outbound connections during the test suite and **is not a production control**; an earlier draft of
+the assurance case described it as one, and that was corrected.
+
+**Closes when:** the fetch tool resolves the host and refuses private, loopback, link-local and
+unique-local destinations unless a deployment opts in, with a test that a redirect to such an
+address is refused as well as a direct request.
+
+### O2. An advisory in a transitive dependency of the desktop shell
+
+**Severity: low, scoped. Class: dependency.** `glib` 0.18.5 carries GHSA-wrw7-89jp-8q8g. It is
+transitive through the GTK stack the desktop framework requires on Linux and is not fixable by a
+direct bump. **Scope was established rather than assumed:** the headless release ships no desktop
+shell, so a server deployment does not carry it. Recorded as R25.
+
+**Closes when:** a desktop framework release resolves `glib` 0.20 or later.
+
+### O3. Releases are not signed
+
+**Severity: medium at first release, none today. Class: distribution.** Every packaged artifact
+carries build provenance attested through the build's own identity and recorded in a public
+transparency log, which establishes where bytes were built. Nothing establishes that the project
+intended to publish them, because the project publishes no releases yet.
+
+**Closes when:** the release pipeline signs artifacts with a project-held key whose private half is
+not on the distribution host, with a documented verification procedure. Until then, publishing a
+release would create the exposure rather than reveal it.
+
+### O4. One reviewer
+
+**Severity: medium. Class: process.** Every commit in the tree has one author. The build is the
+only thing that has reviewed most changes, and a build cannot judge whether a change is worth
+making. Two further maintainers now hold review and merge rights; the history does not yet show
+them using it.
+
+**Closes when:** reviews and merged changes from a maintainer other than the author appear in the
+git history. Recorded as R22.
+
+## What this review does not establish
+
+It does not establish the absence of vulnerabilities, and no review does. It establishes that the
+security requirements were read as claims, that each was traced to the code and the test that hold
+it, that the boundary was walked in both directions, and that the findings above were acted on. The
+next review should begin by checking whether the open items above are still open, and whether the
+five findings stayed fixed.