Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
74 changes: 74 additions & 0 deletions docs/code_review.md
Original file line number Diff line number Diff line change
@@ -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.
114 changes: 114 additions & 0 deletions packages/ballast-typescript/src/docs-links.test.ts
Original file line number Diff line number Diff line change
@@ -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([]);
});
});
9 changes: 5 additions & 4 deletions plans/plan-spec-kit-development-process.md
Original file line number Diff line number Diff line change
@@ -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)_
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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. |
15 changes: 8 additions & 7 deletions tasks/todo.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
Loading