diff --git a/docs/README.md b/docs/README.md index 09863cb5..8c822048 100644 --- a/docs/README.md +++ b/docs/README.md @@ -92,6 +92,12 @@ See [installation.md](installation.md) for package-specific commands, skill inst - go install/go run (`ballast-go`) - installed skills under target-specific skill locations such as `.claude/skills/` and `.opencode/skills/` +## Code Review + +See [code_review.md](code_review.md) for the review policy this repository follows: what +reviews look for, the severity scale and when each level blocks a merge, the gates a change +must pass before review is requested, and how agent-assisted review fits in. + ## Publishing See [publish.md](publish.md) for npmjs, Python GitHub release assets, and Go release publishing setup. diff --git a/docs/code_review.md b/docs/code_review.md new file mode 100644 index 00000000..1eae6611 --- /dev/null +++ b/docs/code_review.md @@ -0,0 +1,74 @@ +# Code Review + +The review policy this repository follows, and that `AGENTS.md` points agent reviewers at. + +Reviews exist to catch defects, not to relitigate style. Formatting is settled by the +formatter and linter; if a review comment could have been a lint rule, it should be one. + +## What a review is looking for + +In priority order: + +1. **Correctness** — does it do what it claims, including on the failure paths? +2. **Security** — secrets, injection, permission and credential scope, dependency risk. +3. **Regressions** — behaviour other code or downstream repositories already rely on. +4. **Missing tests** — especially for the failure paths, not only the happy one. +5. **Maintainability** — will the next person be able to change this safely? + +## Severity + +| Level | Use when | Blocks merge | +| --- | --- | --- | +| P0 | Concrete failure mode with user or data impact | Yes | +| P1 | Credible risk, or a missing test on behaviour that matters | Yes | +| P2 | Real but bounded; fine to follow up under an issue | No | +| P3 / Nit | Polish and preference | No | + +Rules for applying it: + +- Flag P0/P1 only when there is a **concrete failure mode or credible risk**. "This could + be cleaner" is not a risk. +- Treat missing tests as **P1** when the change touches behaviour, auth, billing, + persistence, migrations, concurrency, permissions, or user-visible output. +- Treat documentation gaps as **P1** only when the change alters setup, public APIs, + release or deploy steps, or user-visible behaviour. +- Never block merge on personal preference. + +## Evidence, not assertion + +A review finding should be reproducible by the author from the comment alone. State the +failing input or state, and what goes wrong — not just that something looks wrong. The +same standard applies to the author: a claim that something is fixed should name the test +that failed before and passes now. + +This repository has been bitten specifically by *plausible but unverified* claims. See +[ADR-002](../adr/002-trust-ballasts-own-feedback-channels.md): a check that reports success +without running, or a log that names files it did not write, is the failure mode to watch +for. When a review or a fix rests on "this should be fine now", ask for the evidence. + +## Gates before requesting review + +- The smallest relevant test, lint, or type-check command has been run, and passes. +- Commit-time hooks ran — a change committed with `--no-verify` is not review-ready + without saying so and stating what was run instead. +- Behaviour changes carry a test that failed before the change. +- Docs changed alongside behaviour, CLI, configuration, architecture, or workflow changes. +- For multi-backend changes, **all four install surfaces** were updated together — see the + multi-backend note in `CLAUDE.md`. A TypeScript-only change passes every TypeScript test + while leaving Go, Python and the wrapper broken. + +## Agent reviewers + +Agent-assisted review runs through the `github-pr-copilot-cycle` skill, which requests +review, triages the comments, applies the actionable ones, and repeats for up to three +cycles. See [skills/github-pr-copilot-cycle.md](skills/github-pr-copilot-cycle.md). + +Agent review supplements human judgement. Treat its findings as claims to verify, with the +same evidence bar as any other reviewer — a confidently-worded finding is not a verified +one. + +## Scope discipline + +- Review the change that was proposed, not the change you would have made. +- Out-of-scope problems found during review become issues, not review blockers. +- A large diff is not automatically a problem, and a small diff is not automatically safe. diff --git a/packages/ballast-typescript/src/docs-links.test.ts b/packages/ballast-typescript/src/docs-links.test.ts new file mode 100644 index 00000000..1698a84f --- /dev/null +++ b/packages/ballast-typescript/src/docs-links.test.ts @@ -0,0 +1,114 @@ +import fs from 'fs'; +import path from 'path'; + +const REPO_ROOT = path.resolve(__dirname, '../../..'); + +/** + * The hand-written documentation surface. Generated output under `.claude/` and + * `.codex/` is excluded: it is rebuilt from `agents/` and already covered by + * repo-generated-artifacts.test.ts, and the packaged copies under `packages/` + * are synced rather than authored. + */ +const DOC_ROOTS = ['docs', 'adr', 'plans', 'tasks']; +const DOC_FILES = ['README.md', 'AGENTS.md', 'CLAUDE.md']; + +function collectMarkdown(): string[] { + const found: string[] = []; + + for (const file of DOC_FILES) { + if (fs.existsSync(path.join(REPO_ROOT, file))) { + found.push(file); + } + } + + const walk = (relativeDir: string): void => { + const absolute = path.join(REPO_ROOT, relativeDir); + if (!fs.existsSync(absolute)) { + return; + } + for (const entry of fs.readdirSync(absolute, { withFileTypes: true })) { + const relative = path.join(relativeDir, entry.name); + if (entry.isDirectory()) { + walk(relative); + } else if (entry.name.endsWith('.md')) { + found.push(relative); + } + } + }; + + DOC_ROOTS.forEach(walk); + return found.sort(); +} + +/** A path containing placeholder syntax is an example, not a reference. */ +function isTemplate(target: string): boolean { + return /[<>*{}]|\.\.\./.test(target); +} + +describe('documentation links', () => { + const files = collectMarkdown(); + + test('finds the documentation surface it is meant to guard', () => { + expect(files).toContain('AGENTS.md'); + expect(files).toContain('docs/README.md'); + expect(files.length).toBeGreaterThan(10); + }); + + test('every relative markdown link resolves', () => { + const broken: string[] = []; + + for (const file of files) { + const body = fs.readFileSync(path.join(REPO_ROOT, file), 'utf8'); + for (const match of body.matchAll(/\[[^\]]*\]\(([^)]+)\)/g)) { + const target = match[1].split('#')[0].trim(); + if ( + target === '' || + /^[a-z][a-z0-9+.-]*:/i.test(target) || + target.startsWith('/') || + isTemplate(target) + ) { + continue; + } + const resolved = path.resolve( + path.dirname(path.join(REPO_ROOT, file)), + target + ); + if (!fs.existsSync(resolved)) { + broken.push(`${file} -> ${target}`); + } + } + } + + expect(broken).toEqual([]); + }); + + test('every docs/ path named in backticks exists', () => { + // #340: AGENTS.md said "Follow `docs/code_review.md` for code reviews" for a + // file that did not exist. A markdown-link check would not have caught it, + // because the reference was inline code. Scoped to docs/*.md so that + // placeholder paths elsewhere are not mistaken for broken references. + // + // plans/ and tasks/ are excluded on purpose: they describe intent, and a + // plan or todo naming a file it intends to create is correct, not drift. + // Everything else here asserts what the repository has now, and must be + // true -- README.md, AGENTS.md, CLAUDE.md, docs/ and adr/. + const broken: string[] = []; + const assertsCurrentState = (file: string): boolean => + !file.startsWith('plans/') && !file.startsWith('tasks/'); + + for (const file of files.filter(assertsCurrentState)) { + const body = fs.readFileSync(path.join(REPO_ROOT, file), 'utf8'); + for (const match of body.matchAll(/`(docs\/[A-Za-z0-9_./-]+\.md)`/g)) { + const target = match[1]; + if (isTemplate(target)) { + continue; + } + if (!fs.existsSync(path.join(REPO_ROOT, target))) { + broken.push(`${file} -> ${target}`); + } + } + } + + expect(broken).toEqual([]); + }); +}); diff --git a/plans/plan-spec-kit-development-process.md b/plans/plan-spec-kit-development-process.md index 266c330f..6dab73da 100644 --- a/plans/plan-spec-kit-development-process.md +++ b/plans/plan-spec-kit-development-process.md @@ -1,6 +1,6 @@ # Plan: Spec Kit Development Process -**Status:** Proposed +**Status:** In progress (Phases 2 and 4 complete; Phase 1 blocked on the placement question) **Branch:** docs/spec-kit-development-process **Created:** 2026-08-29 **Related ADRs:** _(none yet)_ @@ -93,7 +93,7 @@ This example was verified against the repository's `.rulesrc.json` on 2026-09-16 - `docs/agents/spec-kit.md` - exists; add cross-links from the agent guide into this process. - `docs/skills/speckit-bootstrap.md`, `docs/skills/speckit-reverse-engineer.md`, `docs/skills/speckit-delivery.md` - exist; each covers its own procedure, so this process should link them rather than restate them. - `docs/skills/github-pr-copilot-cycle.md` - exists; link it as the PR closure gate. -- `docs/code_review.md` - **missing but referenced** by `AGENTS.md` ("Follow `docs/code_review.md` for code reviews"). Either create it or fix the reference before treating it as a process gate. +- `docs/code_review.md` - **created** (#340). `AGENTS.md` now points at a real review policy, so it can be treated as a process gate. ## Process Model @@ -228,9 +228,9 @@ A change is complete only when: Per-agent and per-skill guides (`docs/agents/spec-kit.md`, `docs/skills/speckit-*.md`, `docs/skills/github-pr-copilot-cycle.md`) already exist from the Spec Kit merge, so the remaining work is the connective tissue between them, not new per-artifact docs. - [ ] Phase 1: Resolve the placement question, then write the end-to-end process doc that sequences the existing guides. -- [ ] Phase 2 (#340): Fix the broken `docs/code_review.md` reference in `AGENTS.md` (create the doc or repoint the reference). +- [x] Phase 2 (#340): Created `docs/code_review.md` rather than repointing. The reference is written into `AGENTS.md` by an external Codex reviewer installer (an orphaned `END CODEX REVIEWER INSTALLER` marker sits below it), so providing the target is durable where editing the pointer would be overwritten. Indexed in `docs/README.md`. - [ ] Phase 3: Add cross-links from the existing Spec Kit guides to task, plan lifecycle, testing, docs, and PR review guidance. -- [ ] Phase 4: Add a docs-link check that prevents advertised-process drift (would have caught the `docs/code_review.md` break). +- [x] Phase 4: Added `packages/ballast-typescript/src/docs-links.test.ts`. Checks that every relative markdown link resolves, and that every `docs/*.md` path named in backticks exists. Verified it reproduces #340 by deleting the doc. `plans/` is excluded from the second check, since a plan naming a file it intends to create describes intent, not current state. - [ ] Phase 5: Run focused validation and update this plan with evidence. ## Verification @@ -270,3 +270,4 @@ Future implementation should verify: | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | 2026-08-29 | Plan created with Spec Kit process model and target `.rulesrc.json` configuration. | | 2026-09-16 | Merged main; corrected Files Affected (Spec Kit agent/skill docs already exist) and rescoped phases to the remaining connective work; confirmed the `.rulesrc.json` example still matches the repository. | +| 2026-09-29 | Phases 2 and 4 landed. Phase 1 still needs the placement question answered. | diff --git a/tasks/todo.md b/tasks/todo.md index f56d38ce..f0506d10 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -51,13 +51,14 @@ A stale plan is worse than no plan — it is read as current intent. via #339/#342/#325, leaving only #128 and #94, which are tracked as issues. The Phase 2 and Phase 3 design notes, and the unanswered parity open question, were posted to #128 and #94 before deletion so nothing was lost. -- [ ] **`plans/plan-spec-kit-development-process.md` — decide whether to pursue.** Proposed - 2026-08-29, a month old with no work started and four open questions unresolved. - Related issue #303 is still open. Either commit to it and answer the questions, or - retire it and record why on #303. -- [ ] Note while triaging: that plan's open question *"should `docs/code_review.md` be - created, or should `AGENTS.md` point to an existing review document?"* is live issue - **#340**, filed separately on 2026-09-17. Answer it once, in one place. +- [ ] **`plans/plan-spec-kit-development-process.md` — kept and in progress.** Phases 2 + and 4 are done; Phase 1 is blocked on its placement question (does the end-to-end + process doc live at `docs/development-process.md`, under `docs/agents/spec-kit.md`, + or both?). Phases 3 and 5 follow Phase 1. +- [x] **#340 answered and closed.** The spec-kit plan's open question and #340 were the + same question. Resolved by creating `docs/code_review.md` rather than repointing + `AGENTS.md`: that line is written by an external Codex reviewer installer, so + supplying the target is durable where editing the pointer is not. - [ ] Promote whichever of these outlive this branch into GitHub issues and record the links here, per the task-system rule. `tasks/todo.md` is not durable tracking.