-
-
Notifications
You must be signed in to change notification settings - Fork 433
docs: add an AI policy and agent instructions #5377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
marcoscaceres
wants to merge
26
commits into
main
Choose a base branch
from
docs/ai-policy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
5215a63
docs: add an AI policy
marcoscaceres c9ce3d2
docs(ai-policy): split audiences, add responsibility clause, scope th…
marcoscaceres b47d17e
docs(ai-policy): the two-model process is ours, not a contributor req…
marcoscaceres 9692abf
docs(ai-policy): state the house style for PR bodies, commits, and co…
marcoscaceres 3a9b72b
docs(ai-policy): resolve the brevity vs issue-accounting conflict
marcoscaceres f37ab08
docs(ai-policy): fix the labeling mechanism, add a privacy rule, scop…
marcoscaceres 992b930
docs: point contributors, human and otherwise, at the AI policy
marcoscaceres fc46573
docs: show the mascot on CONTRIBUTING, keep both at a sane size
marcoscaceres f170b03
docs(ai-policy): name the work we keep away from an agent
marcoscaceres 4b75bc5
chore: add Copilot repository instructions and agent setup steps
marcoscaceres bd1c0eb
chore(copilot): keep the browser download, put the build in the canon…
marcoscaceres a4214f6
chore(copilot): require BROWSERS, keep the browser in the workspace, …
marcoscaceres 1241754
fix(copilot): install the browser explicitly, not via puppeteer's pos…
marcoscaceres a2ac8ec
fix(copilot): set the puppeteer cache per step, since job env is not …
marcoscaceres 4976f34
chore(eslint): ignore the in-workspace puppeteer cache
marcoscaceres d261f36
docs(copilot): note the workspace browser cache is not for humans
marcoscaceres 59a59e8
chore(copilot): dynamic import in the browser probe, shorter image al…
marcoscaceres 9023d2f
docs(ai-policy): welcome Co-Authored-By for the model that wrote it
marcoscaceres 2b9664c
docs: commit a CLAUDE.md, keeping personal notes in CLAUDE.local.md
marcoscaceres 4cca610
docs: drop a dead pointer and un-stale the headings API note
marcoscaceres ca58627
docs: drop two CI notes that no longer hold
marcoscaceres d49d965
docs: soft wrap, and fix four things Sid caught
marcoscaceres 2913f47
docs: require proof that a bug reproduced before the fix
marcoscaceres 57ab72a
Merge branch 'main' into docs/ai-policy
marcoscaceres 332d14f
Apply suggestions from code review
marcoscaceres f954f27
Apply suggestion from @marcoscaceres
marcoscaceres File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| # Copilot instructions for ReSpec | ||
|
|
||
| ReSpec is a browser-based tool that generates W3C specifications. Authors write HTML plus a `<script>` tag, and ReSpec fills in boilerplate, cross-references, bibliography, and validation. Roughly half of W3C standards are produced with it, so a regression here reaches a lot of documents. | ||
|
|
||
| Contributions written with AI are welcome and have a policy: see [AI_POLICY.md](../AI_POLICY.md). Read it before opening a pull request. | ||
|
|
||
| ## Build and test | ||
|
|
||
| This is the whole sequence. The build is part of it, and so are `BROWSERS` and `PUPPETEER_CACHE_DIR`: | ||
|
|
||
| ```bash | ||
| export PUPPETEER_CACHE_DIR="$PWD/.cache/puppeteer" | ||
| pnpm i --frozen-lockfile | ||
| pnpm lint # tsc -p src/jsconfig.json && eslint . | ||
| pnpm build:w3c && pnpm build:geonovum && pnpm build:aom && pnpm build:dini | ||
| BROWSERS=ChromeHeadless pnpm test # unit then integration, via karma | ||
| pnpm test:build # the builder tool | ||
| pnpm test:headless # renders examples through puppeteer | ||
|
marcoscaceres marked this conversation as resolved.
|
||
| ``` | ||
|
|
||
| **Always set `BROWSERS`, or pass `--browsers`.** No karma config sets a default, so `pnpm test` on its own launches nothing, waits for a browser to connect by hand, and hangs until something kills it. Nothing in the output says so; it simply stops after printing `START:`. | ||
|
|
||
| **Set `PUPPETEER_CACHE_DIR` to `$PWD/.cache/puppeteer` for anything that launches a browser**, which includes `pnpm test:headless` via `tools/respecDocWriter.js`. The agent environment provisions the browser there rather than in the default home cache, so puppeteer will not find it unless pointed at the same place. | ||
|
|
||
| If you are a human with a browser already in puppeteer's default cache, you do not need that line, and you should skip it: the workspace copy is around 340 MB and does not get shared between checkouts. | ||
|
|
||
| The integration suite (`tests/spec/`) reads the bundles in `builds/`, not `src/`, so a source change has no effect on it until the bundle is rebuilt. That is why the build sits above the test line rather than being mentioned afterwards. The unit suite (`tests/unit/`) loads `src/` directly, so it needs no rebuild and is the faster loop while iterating. | ||
|
|
||
| There is one bundle per profile. Building only `w3c` leaves the Geonovum, DiNI and AOM suites testing the previous code, which looks like a passing or failing test that has nothing to do with the change. If a change that provably does nothing alters a test result, suspect a stale bundle before suspecting the test. | ||
|
|
||
| Never commit anything under `builds/`. CI rebuilds it, and a PR touching it fails a dedicated check. | ||
|
|
||
| To run a single integration suite, rebuild first, then pass the describe block. The unit config needs no rebuild: | ||
|
|
||
| ```bash | ||
| pnpm build:w3c | ||
| npx karma start tests/spec/karma.conf.cjs --single-run --browsers ChromeHeadless --grep="Core - Inlines" | ||
| npx karma start tests/unit/karma.conf.cjs --single-run --browsers ChromeHeadless | ||
| ``` | ||
|
|
||
| `--grep` is a literal string match. Alternation does not work, and only the last `--grep` counts, so run separate invocations instead. | ||
|
|
||
| ## Code style | ||
|
|
||
| Prefer functional style over imperative loops: `forEach`, `map`, `filter`, `find`, `reduce`, `some`, `every`. Use an early `return` inside `forEach` rather than `continue`. `NodeList`, `Map` and `Set` have `forEach` natively, so prefer `nodeList.forEach()` over `[...nodeList].forEach()` and avoid the extra array. | ||
|
|
||
| Every `querySelector`, `closest`, `getElementById` and `getAttribute` result is possibly null; check before use. Do not paper over it with `?? ""`, because the empty string then flows on into an `html` template and renders as an empty text node, so the bug shows up later as missing output rather than as a null failing where it happened. | ||
|
|
||
| Write en-US English everywhere, including comments and identifiers: behavior, color, license, center, analyze, initialize, serialize. Leave existing en-GB spelling alone when it is load-bearing, such as an established identifier or a verbatim quotation. | ||
|
|
||
| Run `pnpm format --write` on changed files. CI fails on unformatted code. | ||
|
|
||
| ## Pull requests, commits, and comments | ||
|
|
||
| The house style is in [AI_POLICY.md](../AI_POLICY.md) under "Write it the way the project writes it". In short: | ||
|
|
||
| - Pull request titles follow conventional commits, `type(scope): summary`, enforced by the Check PR Title workflow. | ||
| - Pull request descriptions are plain prose. No `## Summary`, no `## Changes`, no `## Test plan`, no emoji, no bold section labels. A closing reference goes on the first line. | ||
| - `Closes #N` only when every ask in the issue is delivered, otherwise `Refs #N`. Confirm the number is an issue and not a pull request before citing it. | ||
| - Commit messages are one imperative subject line, lowercase after any prefix, no trailing period, and no body unless the diff cannot be understood without one. A `Co-Authored-By` trailer naming the model that wrote it is welcome. | ||
| - Comments say why, not what. Delete a comment that restates the line below it. No bare `fixes #1234` in source. | ||
|
|
||
| ## Tests | ||
|
|
||
| A test that cannot fail is worse than no test, because it costs review time and implies coverage that does not exist. A regression test must fail on `main` for the reason the issue describes, and pass with the fix. Do not assert a literal the implementation just set, and do not exercise a path the issue never mentioned while the reported path stays uncovered. | ||
|
|
||
| Do not claim a bug is fixed without showing it broken first. Run the same repro against the pre-fix state and against your fix, and put that in the PR description. Prefer running it on the base commit; when the base will not build with your new test, revert only the fix and say so. The exact format the check validates is in [ai-pr-proof-requirement.md](../ai-pr-proof-requirement.md). | ||
|
|
||
| One concern per pull request. A locale addition or a drive-by refactor belongs in its own PR even when it is one line and obviously correct. | ||
|
|
||
| ## Adding a module | ||
|
|
||
| A module goes in `src/core/` only if every profile wants it. If it is specific to one, put it in that profile's folder instead, as `src/w3c/` already does for eight modules. Either way it exports `name` and a `run(conf)`. `run` may be synchronous or async, whichever the work needs: most core modules are synchronous, and only the ones that fetch or await something are not. The module must be registered in every profile that needs it: `profiles/w3c.js`, `profiles/geonovum.js`, `profiles/aom.js`, `profiles/dini.js`. Tests go in `tests/spec/core/`. If the module uses `getIntlData`, add a Czech (`cs`) entry. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| name: "Copilot Setup Steps" | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| push: | ||
| paths: | ||
| - .github/workflows/copilot-setup-steps.yml | ||
| pull_request: | ||
| paths: | ||
| - .github/workflows/copilot-setup-steps.yml | ||
|
|
||
| jobs: | ||
| copilot-setup-steps: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| steps: | ||
| - uses: actions/checkout@v7 | ||
| - uses: pnpm/action-setup@v5 | ||
| - uses: actions/setup-node@v6 | ||
| with: { node-version-file: ".nvmrc", cache: pnpm } | ||
|
|
||
| # Deliberately NOT setting PUPPETEER_SKIP_DOWNLOAD, though the CI workflows | ||
| # do. CI can skip the download because it points PUPPETEER_EXECUTABLE_PATH at | ||
| # the runner's Chrome for the same job. Variables set here do not reach the | ||
| # agent's later commands, so skipping would leave it with neither a bundled | ||
| # browser nor a path to one. | ||
| # | ||
| # PUPPETEER_CACHE_DIR is set per step, not on the job. Copilot honors only | ||
| # steps, permissions, runs-on, services, snapshot and timeout-minutes, so a | ||
| # job-level env block is dropped when it runs these as setup steps, even | ||
| # though a normal Actions run would apply it. The workspace location is | ||
| # deliberate: puppeteer defaults to ~/.cache/puppeteer, and the home | ||
| # directory is not guaranteed to survive into the agent's own session. | ||
| - run: pnpm i --frozen-lockfile | ||
| env: | ||
| PUPPETEER_CACHE_DIR: ${{ github.workspace }}/.cache/puppeteer | ||
|
|
||
| # Explicitly, rather than relying on puppeteer's postinstall. pnpm caches the | ||
| # fact that a build script ran, but the browser that script downloads lands | ||
| # outside node_modules, so on a warm store the install completes in seconds | ||
| # and no browser appears. This command is idempotent. | ||
| - run: npx puppeteer browsers install chrome | ||
| env: | ||
| PUPPETEER_CACHE_DIR: ${{ github.workspace }}/.cache/puppeteer | ||
|
|
||
| # karma loads the bundles from builds/, not src/, and there is one per | ||
| # profile. Building all four means the agent can run any suite without first | ||
| # working out why a src/ change had no effect on the tests. | ||
| - run: pnpm build:w3c | ||
| - run: pnpm build:geonovum | ||
| - run: pnpm build:aom | ||
| - run: pnpm build:dini | ||
|
|
||
| # Fails the setup loudly if puppeteer has no usable browser, rather than | ||
| # leaving the agent to discover it mid-task. Same cache dir as the install, | ||
| # or this would check a location nothing populated. Dynamic import rather | ||
| # than require, so it does not depend on require(esm) support if the pinned | ||
| # Node version ever moves. | ||
| - name: Check puppeteer has a browser | ||
| env: | ||
| PUPPETEER_CACHE_DIR: ${{ github.workspace }}/.cache/puppeteer | ||
| run: | | ||
| node -e " | ||
| const fs = require('fs'); | ||
| import('puppeteer') | ||
| .then(p => p.default.executablePath()) | ||
| .then(p => { fs.accessSync(p); console.log('browser ok:', p); }) | ||
| .catch(e => { console.error('no usable browser:', e.message); process.exit(1); }) | ||
| " |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| # AI policy | ||
|
|
||
| ReSpec accepts contributions written with AI. This document says what we ask for in return. | ||
|
|
||
| <img src="assets/ai-is-welcome-here.png" alt="Pixel-art robot grinning in front of a burning forest, captioned "AI is welcome here."" width="320"> | ||
|
|
||
| Everything under "Everyone" applies to any contributor, maintainers included, and it is short. The section after it describes the stricter process we run on our own AI generated work, and asks nothing of you. | ||
|
|
||
| ## Everyone | ||
|
|
||
| ### You are responsible for what you submit | ||
|
|
||
| By opening a pull request you are stating that you understand every line of it, that you have run it, and that you have the right to contribute it under the project's license. Responsibility for correctness, security, and copyright sits with you, not with the tool you used. We will not accept "the AI wrote it" as an account of a defect. | ||
|
|
||
| Two specific things to check before submitting, because generated code gets them wrong in ways that read as plausible: that every package you import actually exists and is one we already depend on or that you are deliberately adding, and that every DOM or platform API you call is real. Invented dependencies and invented APIs are the two failure modes we see most. | ||
|
|
||
| ### Disclose it | ||
|
|
||
| If AI generated any of the logic in your contribution, say so in the pull request description. Generated logic means an agent, a function, an algorithm, a test, a feature: something you would otherwise have had to work out. | ||
|
|
||
| You do not need to apply a label. Labeling requires triage access on this repository, which most contributors do not have, so a maintainer adds the `AI` label based on what your description says. One line is enough. | ||
|
|
||
| Editor autocomplete, a rename refactor, or a model helping you word a comment do not need disclosing. The point is to tell a reviewer where to spend attention, not to tally tool use, and a disclosure on everything tells them nothing. | ||
|
|
||
| ### Do not paste private material into a model | ||
|
|
||
| Whatever you send to a hosted model leaves this project, and may be retained or used for training. Do not paste into one: credentials or tokens, an embargoed or unpublished security report, a member-confidential or otherwise private W3C document, or third-party code or text you do not have the right to redistribute. | ||
|
|
||
| This matters more here than in most projects, because the people writing specs with ReSpec routinely handle material that is under embargo or restricted to group members. If a bug can only be explained with such material, describe the shape of the problem instead, or ask a maintainer to reproduce it. | ||
|
|
||
| ### One concern per pull request | ||
|
|
||
| A PR fixes one thing. A locale addition, a drive-by refactor, or a second bug fix belongs in its own PR, even when it is a one line change and even when it is obviously correct. Bundling is the most common reason an AI PR takes three review rounds instead of one. | ||
|
|
||
| ### Write it the way the project writes it | ||
|
|
||
| Generated prose has a house style of its own, and it is not ours. Three places it shows up: | ||
|
|
||
| **The pull request description** is plain prose. No `## Summary`, no `## Changes`, no `## Test plan`, no emoji, no bold section labels. If it closes an issue, that line goes first. Then say how the bug was fixed, then anything else a reviewer needs. Two or three sentences is usually enough for that part. A sentence saying what you ran locally is welcome; a formal test-plan section is not, since CI runs the tests and the reviewer can read them. | ||
|
|
||
| **The commit message** is one imperative subject line, lowercase after any prefix, no trailing period. A body only when the diff genuinely cannot be understood without one, and then a sentence or two. No bullet lists, no "Summary", no recap of what the diff already shows. A `Co-Authored-By` trailer naming the model that wrote it is welcome, and consistent with what already appears in this project's history. | ||
|
|
||
| **Code comments** say why, not what. A comment restating the line below it is noise a reader has to skim past, and generated code produces a lot of it. The test is not whether a comment is obvious to us, which you have no way to judge: it is whether the comment would still be needed if the reader could see the code, and they can. What earns a comment is a reason that is not visible: a workaround for a browser bug, an ordering constraint, why the obvious approach fails. The same test applies to an issue link: keep it when the issue records that reason, drop it when it does not. A bare `fixes #1234` in the source is the common failing case, since it tells a future reader nothing the history does not already hold. | ||
|
|
||
| ### Say exactly what the issue asked for | ||
|
|
||
| Say which of the issue's asks you addressed and which you did not. A short list is fine and does not count against keeping the prose brief; the point is that a reviewer can tell at a glance whether the issue is finished, not that you produce a formal document. | ||
|
|
||
| Only write `Closes #N` when every ask is delivered. Otherwise write `Refs #N` and say what is left. Before citing a number, confirm it is an issue rather than a pull request, and that it is open. | ||
|
|
||
| ### What does not count as a test | ||
|
|
||
| This applies to every contribution, from anyone, wherever a test is offered as evidence that a fix works. Not every change needs a regression test: a refactor, a docs change, or a dependency bump may need none. But a test that claims to pin a bug and cannot fail proves nothing and costs review time. We reject: | ||
|
|
||
| - a test that passes on `main` without the fix, when it is offered as proof of one | ||
| - a test asserting a literal the implementation just set, or restating the implementation's own shape | ||
| - a test exercising a path the issue never mentioned, while the reported path stays uncovered | ||
| - a test whose name claims more than its body checks | ||
|
|
||
| ### Screenshots when the change is visual | ||
|
|
||
| If the change alters rendered output, include before and after screenshots. If it does not, skip them. Most AI PRs here are not visual and screenshots would prove nothing about them. | ||
|
|
||
| ### Prove the bug is fixed | ||
|
|
||
| A bug-fix PR has to show the bug happening before the change and not happening after it, using the same repro both times. A green test proves nothing on its own if nobody saw it fail first. For a rendering bug, add before and after images; for everything else the repro output is the proof, and images are optional. The machine-readable form the automated check looks for is specified in [ai-pr-proof-requirement.md](ai-pr-proof-requirement.md); keep the description itself plain prose. | ||
|
|
||
| ### Say what you did not fix | ||
|
|
||
| List every review finding you chose not to act on, with your reason, in the PR before asking for another round. A finding you drop silently is a decision you made on the reviewer's behalf. | ||
|
|
||
| ## What the maintainers hold themselves to | ||
|
|
||
| Nothing in this section is asked of you. It describes how we run our own AI generated work, and it is here so you can see the standard we are applying to ourselves rather than only to contributions. | ||
|
|
||
| If you are contributing, you need one model and your own judgment. You are never expected to own a second AI subscription, or to orchestrate models against each other, to send us a patch. | ||
|
|
||
| ### The test is written by a different model than the fix | ||
|
sidvishnoi marked this conversation as resolved.
|
||
|
|
||
| A model that writes a fix cannot be trusted to write the test for it, because a test authored alongside an implementation tends to assert what the code happens to do rather than what the report said was broken. So, in our own pipeline: | ||
|
|
||
| 1. The test is written by a **different** model from the one writing the fix, and is written **from the issue alone, before the fix exists**. It has to fail on `main` for the reason the issue describes. Starting red is what makes passing later mean something. | ||
| 2. The fix is then written to make that test pass. | ||
| 3. A third model audits both, judging whether the test pins the reported behavior or something incidental. Every finding gets answered: fixed, or a reason why it is wrong. No thread is resolved silently. | ||
|
|
||
| In practice that means one of Claude or Gemini writes the test and the other writes the fix, and Copilot audits both. This is enforceable for us only because we control the whole pipeline. From outside a pull request, nobody could check that the ordering happened, which is exactly why we do not ask it of anyone else. | ||
|
|
||
| ### Higher scrutiny, on purpose | ||
|
|
||
| We hold this work to a stricter standard than a human's. A model can produce a confident, fluent, well formatted pull request that is wrong, and can do it faster than anyone can read it. That asymmetry is the reason for the extra steps. | ||
|
|
||
| We would rather a model attempt a real fix under these checks than a timid one, so most of the hard bugs are fair game. Some are not, and this matches GitHub's own guidance on what to keep away from a coding agent: security, authentication and anything touching personal data, a production-critical breakage, and changes resting on substantial business logic or on design consistency across the codebase. Those we do ourselves. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.