-
-
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
base: main
Are you sure you want to change the base?
Changes from 19 commits
5215a63
c9ce3d2
b47d17e
9692abf
3a9b72b
f37ab08
992b930
fc46573
f170b03
4b75bc5
bd1c0eb
a4214f6
1241754
a2ac8ec
4976f34
d261f36
59a59e8
9023d2f
2b9664c
4cca610
ca58627
d49d965
2913f47
57ab72a
332d14f
f954f27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| # 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. | ||
|
|
||
| Karma reads the bundles in `builds/`, not `src/`. A source change has no effect on | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: Except for unit tests - which use src directly. Faster flow. |
||
| the tests until the bundle is rebuilt, which is why the build sits above the test | ||
| line rather than being mentioned afterwards. | ||
|
|
||
| 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 suite, rebuild first, then pass the describe block. Integration and | ||
| unit suites use different configs: | ||
|
|
||
| ```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 `npx prettier --write` on changed files. CI fails on unformatted code. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
marcoscaceres marked this conversation as resolved.
Outdated
|
||
|
|
||
| ## 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 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also mention the pull request title format. |
||
| trailing period, and no body unless the diff cannot be understood without one. | ||
| Never add `Co-Authored-By` for a model. | ||
| - 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. | ||
|
|
||
| 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 new module under `src/core/` exports `name` and a `run(conf)`. `run` may be | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a module is meant for only certain profile, add to profile folder instead of core. |
||
| 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. | ||
| 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 | ||
|
marcoscaceres marked this conversation as resolved.
|
||
| 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); }) | ||
| " | ||
|
marcoscaceres marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| # 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. | ||
|
|
||
| ### 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. | ||
Uh oh!
There was an error while loading. Please reload this page.