From 6b6ad4a648d47b4625a5c855d00e2eb36799a36b Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Tue, 6 Oct 2026 17:28:21 -0400 Subject: [PATCH 1/2] docs: the code review standard docs/CODE_REVIEW.md says how a pull request is reviewed and what it needs to merge: CI's required checks first, a reviewer who isn't the author, every thread resolved, and a checklist of what must be true (spec and tests, trust boundaries, input validation, privacy inventory, dependencies, every-language and accessibility rules, size budgets, docs, DCO), plus the standard of evidence for claims. It also says plainly that the lead maintainer may still merge their own change after CI while there are few maintainers. CONTRIBUTING.md links it. This makes the OpenSSF gold criterion code_review_standards Met. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ayla Croft --- CONTRIBUTING.md | 6 ++- docs/CODE_REVIEW.md | 89 ++++++++++++++++++++++++++++++++++++++++++ docs/best-practices.md | 2 +- 3 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 docs/CODE_REVIEW.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f950085..255d1c0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,8 +72,10 @@ site in `site/` (its own `package.json`, build and tests), in CodeQL) for security problems. What fails a pull request, the licenses a new dependency may have, and how to accept a false positive are in the [dependency and static analysis policy](docs/security/dependency-and-static-analysis-policy.md). -- Maintainers review every pull request; how decisions are made and who maintains what is - in [GOVERNANCE.md](GOVERNANCE.md) and [MAINTAINERS.md](MAINTAINERS.md). +- Maintainers review every pull request against [docs/CODE_REVIEW.md](docs/CODE_REVIEW.md): + what a reviewer checks and what a change needs before it merges. How decisions are made + and who maintains what is in [GOVERNANCE.md](GOVERNANCE.md) and + [MAINTAINERS.md](MAINTAINERS.md). - Commit messages and pull request titles follow [Conventional Commits](https://www.conventionalcommits.org/): `feat(extension): ...`, `fix(server): ...`. Scopes: `server`, `extension`, `spec`, `docs`, `ci`, `release`. diff --git a/docs/CODE_REVIEW.md b/docs/CODE_REVIEW.md new file mode 100644 index 0000000..c2aa4b8 --- /dev/null +++ b/docs/CODE_REVIEW.md @@ -0,0 +1,89 @@ +# Code review + +Last reviewed: 2026-10-06. + +How a change to Kotiko is reviewed: who reviews, what they check, and what a change needs +before it merges. It applies to every pull request, from a maintainer, a contributor or an +agent. The rules contributors follow while writing a change are in +[CONTRIBUTING.md](../CONTRIBUTING.md); this page is the reviewer's side. + +## How a review happens + +1. **CI first.** A reviewer starts once the required checks on `main` are green. They are + listed in the `main` ruleset and include the server and extension test suites, lint, + e2e, the shared spec, the secret, DCO and file checks, the dependency scan and CodeQL. + A red check is the author's to fix, not the reviewer's to wave through. +2. **One reviewer who isn't the author** reads the whole diff on GitHub and leaves a + review: *Approve*, or *Request changes* with each problem as a comment on its line. + While Kotiko has few maintainers, the lead maintainer may merge their own change after + CI passes. GOVERNANCE.md and MAINTAINERS.md say who the maintainers are, and the aim + is that every change gets a second person's review. +3. **Every review thread is resolved** before merging (the ruleset enforces this), + either by a change or by a reply the reviewer accepts. +4. **Merge** with a merge commit. The author or a maintainer merges; nobody bypasses the + ruleset. + +## What the reviewer checks + +Each item is a question the change must answer yes to, or explain why not. + +**It does what it says.** +- The pull request says what changed and why, links its slice or issue, and the diff + matches that description. Nothing unrelated is mixed in. +- The behaviour matches the slice's spec. A deviation is written in the spec's + Implementation notes. + +**It is tested.** +- New or changed behaviour has tests in the same pull request. A bug fix has a regression + test that fails without the fix: check it by reading the test against the fix. +- Tests wait for conditions, never fixed sleeps, and never reach the network or a real + local service. +- The coverage gates still pass (90 % of lines, 80 % of the extension's branches). + +**It keeps users safe.** +- **Trust boundaries hold.** Content scripts never see keys, tokens or where requests go. + Only Kotiko's own pages change the server or lookup destination. Every message type + says who may send it (`lib/messages.js`). +- **Input is validated** where it enters, against an allowlist, with its limits. The + server's checks are in `docs/security/assurance-case.md` section 6. +- **No HTML from data:** text goes in with `textContent`, never `innerHTML` (ESLint also + enforces this). +- **Privacy:** page text never leaves the device, only the word goes to Wiktionary, and + every new network call, host or permission is in `docs/privacy/inventory.md` (a test + checks this). +- No secret or user text reaches a log above debug level. +- A change to authentication, a trust boundary or an input updates the + [assurance case](security/assurance-case.md) and, if users can tell, the + [security requirements](security/requirements.md). +- A new dependency meets the license rule and the + [dependency policy](security/dependency-and-static-analysis-policy.md). The extension + gets no runtime npm code. + +**It works for every learner.** +- Nothing assumes English or one base language. Shared tables are in + `spec/lang/_generic/`, with per-language entries only where the default visibly fails. +- Every UI string is in `extension/_locales/en`. +- Accessibility holds: keyboard, focus and contrast in both themes (the axe and contrast + checks run in CI). +- The popup's size and first-paint budgets pass (`npm run perf`). + +**It is complete.** +- Docs say what changed: the reference docs for routes and settings (a test checks + them), the site's guides, and a plain-language `CHANGELOG.md` line under + `## Unreleased`. +- Every commit is signed off (DCO) and follows Conventional Commits. + +## The standard of evidence + +A claim in a pull request or a review ("fixed", "faster", "not reachable", "no +regression") carries its proof: the test that fails without the change, or the command +that was run and what it printed. "Should work" is not proof. A reviewer who doubts a +claim reruns its proof. Security findings follow the same rule, as in the pre-release +review (slice [54](../slices/54-pre-release-security-review/SPEC.md)). + +## Changes written by agents + +An agent's change is reviewed like anyone else's. When a lead agent hands work to other +agents, the lead also reads every diff, checks each acceptance criterion against a test, +and reruns every check before opening the pull request. A maintainer still decides +whether it merges. diff --git a/docs/best-practices.md b/docs/best-practices.md index 224a881..656a2ea 100644 --- a/docs/best-practices.md +++ b/docs/best-practices.md @@ -276,7 +276,7 @@ earned, and gold once silver is; the answers can be entered before that. | Criterion | Level | Status | Answer | Evidence | Needs | |---|---|---|---|---|---| -| `code_review_standards` | MUST [URL] [N/A J] | Unmet | Partly documented. CONTRIBUTING.md and the pull request template list what every change must have (tests, a regression test for fixes, format and lint clean, docs and the security documents updated, no new runtime dependency in the extension), and main accepts only pull requests whose required checks pass. A code review guide saying how review is conducted, what the reviewer checks and what blocks a merge is planned. | `CONTRIBUTING.md:38-79`; `.github/pull_request_template.md:7-17`; `ls docs` (no CODE_REVIEW.md) | Agent: write `docs/CODE_REVIEW.md` (slice 53 section 8) and link it from CONTRIBUTING.md; answer Met with its URL | +| `code_review_standards` | MUST [URL] [N/A J] | Met | docs/CODE_REVIEW.md documents how code review is done and what is required: CI's required checks pass first; a reviewer who isn't the author reads the whole diff and approves or requests changes line by line; every review thread is resolved before merging; and a checklist of what must be true (does what it says and matches its spec, tested with a regression test for each fix, trust boundaries and input validation hold, no HTML from data, privacy inventory updated, dependencies within the license and dependency policy, no English or single-language assumption, accessibility and size budgets, docs and changelog, DCO sign-off), plus the standard of evidence for any claim. CONTRIBUTING.md links it. https://github.com/ScriptKittyOS/kotiko/blob/main/docs/CODE_REVIEW.md | `docs/CODE_REVIEW.md`; `CONTRIBUTING.md` "Commits and pull requests"; ruleset 24531669 (`required_review_thread_resolution: true`) | – | | `two_person_review` | MUST [J] | Unmet | Not yet. Kotiko has a second maintainer since 2026-10-06, but the merged pull requests so far were checked by their author and CI only. The next steps are reviews by the second maintainer, then an approving review required on main for every change. | `gh pr list --state all --json reviews`; ruleset 24531669 `required_approving_review_count: 0` | Maintainers: review each other's pull requests, then set the main ruleset to 1 required approval | | `build_reproducible` | MUST [URL] [N/A J] | Met | The extension zips, the only built release files, are reproducible by anyone: the build uses entries sorted by path, the commit's timestamp, fixed permissions and a pinned pure-JavaScript deflate, so the bytes don't depend on the machine or time zone. The release workflow builds twice and compares, and docs/reproducible-builds.md shows how to rebuild a release from its tag and check it against the published SHA256SUMS. The server is released as source. https://github.com/ScriptKittyOS/kotiko/blob/main/docs/reproducible-builds.md | `docs/reproducible-builds.md:1-57`; `.github/workflows/release.yml:111-118`; two local builds on 2026-10-06 gave identical SHA-256 | Once v1.0.0 exists, have someone outside the project rebuild it and compare (slice 53 section 7.5) | | `test_invocation` | MUST [URL] | Met | npm test (Node's built-in test runner) for the extension and mix test for the server, the standard commands of each ecosystem; npm run e2e runs the Playwright end-to-end tests. https://github.com/ScriptKittyOS/kotiko/blob/main/CONTRIBUTING.md#setup | `package.json:10-11`; `CONTRIBUTING.md:19-30` | – | From 32abb315393cfedf5f7bad521ae444d9c0f7fbac Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Tue, 6 Oct 2026 17:53:10 -0400 Subject: [PATCH 2/2] docs(best-practices): gold achieve_silver is Met Silver was earned on 2026-10-06, so the gold prerequisite is Met. The page would otherwise have set it back to Unmet. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ayla Croft --- docs/best-practices.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/best-practices.md b/docs/best-practices.md index 22be6f5..398c9ee 100644 --- a/docs/best-practices.md +++ b/docs/best-practices.md @@ -257,7 +257,7 @@ earned, and gold once silver is; the answers can be entered before that. | Criterion | Level | Status | Answer | Evidence | Needs | |---|---|---|---|---|---| -| `achieve_silver` | MUST | Unmet | Not yet. Silver is answered after passing is earned. | bestpractices.dev/projects/15259.json: `achieved_silver_at` null | Passing, then the silver Unmet items above | +| `achieve_silver` | MUST | Met | Kotiko earned the silver badge on 2026-10-06: https://www.bestpractices.dev/projects/15259 | bestpractices.dev entry 15259 (silver 100 %) | – | | `bus_factor` | MUST [URL] | Unmet | Kotiko has two maintainers since 2026-10-06, but every commit so far was written by the lead maintainer, so the bus factor is still 1 in practice. We are working on it: the architecture, the assurance case and the coding standards are written down, every planned piece of work in slices/ is a buildable spec, and GOVERNANCE.md says how the maintainers continue without any one of them. https://github.com/ScriptKittyOS/kotiko/blob/main/docs/governance/continuity.md#bus-factor | `git shortlog -sne HEAD`: one author; `MAINTAINERS.md`; `GOVERNANCE.md` "Continuity" | The second maintainer contributing code and reviews regularly | | `contributors_unassociated` | MUST [URL] | Unmet | Not yet. All significant contributions in the past year come from one person. Issues, Discussions and translations are open to everyone, and small tasks will be labelled for newcomers. | `git shortlog -sne HEAD`: 134 commits, one person; `gh api repos/ScriptKittyOS/kotiko/contributors` gives HackTuah only | Maintainer: recruit; two unassociated significant contributors in a year | | `copyright_per_file` | MUST [J] | Met | Every source code file (Elixir, JavaScript, shell, CSS, HTML, Astro, workflow YAML) starts with SPDX-FileCopyrightText: 2026 ScriptKittyOS and the Kotiko contributors. Files that can't carry a comment (JSON, images), plus Markdown, test fixture pages and two formatter configs, get the same statement through REUSE.toml, following the REUSE specification, and reuse lint runs on every pull request as a required check. | Header check over 352 tracked code files (`git ls-files` *.ex *.exs *.js *.mjs *.sh *.css *.html *.astro *.ts *.mdx): all carry SPDX headers except `server/.formatter.exs`, `server/priv/repo/migrations/.formatter.exs`, `site/src/content/docs/index.mdx` and 24 `test/fixtures/pages/*.html`, all covered by `REUSE.toml:4`; `.github/workflows/ci.yml:141` (`pipx run reuse lint`, required check "versions and licenses") | Agent (optional): put in-file headers on the three non-fixture files too |