diff --git a/.agents/skills/agents-md-review/SKILL.md b/.agents/skills/agents-md-review/SKILL.md new file mode 100644 index 00000000..b9f0aa86 --- /dev/null +++ b/.agents/skills/agents-md-review/SKILL.md @@ -0,0 +1,39 @@ +--- +name: agents-md-review +description: Audit an existing AGENTS.md or CLAUDE.md line by line for wrong, stale, discoverable, duplicated, or content-free guidance and propose the pruned file. Use when asked to review, prune, shrink, or clean up one. +--- + +# AGENTS.md review + +Verdicts apply the test in `agents-md`: more than one cheap feedback round to discover and fix, and stated in no skill or always-read file. + +## Verify, then classify + +Review the file as given and state which revision. +Check each block against reality before judging it; one piece of evidence per verdict is enough. + +- Examples and formats against the tool that consumes them (changelog generator, release tooling, CI workflows): **Wrong**. + Reading the tool's configuration is enough; what it would render wrongly is wrong whether or not the bad output has shipped. +- Lists against the tree and the remote default branch: **Stale**. +- Claimed conventions against merged-PR history: **Never a convention** when nobody follows it and the maintainer confirms nobody decided it. + A maintainer may be introducing the rule deliberately, and a human committer proves nothing about who decided it, so an unfollowed rule the maintainer has not ruled on goes on the human-decision list. +- Rules against the installed skills and always-read files: **Duplicated**. +- Everything else against the test above: **Discoverable**, **Content-free**, or **Plain command**. +- An always-loaded pointer to a long guide: **Misplaced**; it belongs in the skill that needs it. + +A block can earn more than one verdict, each with its own evidence; lead with the one that needs no human confirmation, so pruning can start before the maintainer answers. + +Also look for what is **Missing**: policy the organisation applies elsewhere (intent-file rules, writing conventions, comment markers), non-standard layouts with no pointer, and an `AGENTS.md` with no sibling `CLAUDE.md` symlink. +`agents-md/references/before-after.md` is one repository's worked answer; re-verify every row against the repository in front of you, never paste it. + +## Report + +```md +| Block | Verdict | Evidence | +|---|---|---| +| `Issue: #456`, `Issue: N/A` as valid footers | Wrong, Never a convention | `cliff.toml` turns every `Issue:` value into a Jira link; maintainer confirms neither was decided | +| Tests drive the CLI binary, not the library; see `testsuite/AGENTS.md` | Keep | redoing a library-level testsuite is more than one round; no skill states it | +``` + +Then propose the full pruned file, list what moved to a skill or a nested `AGENTS.md`, and list what needs a human decision. +Do not apply the changes unless asked. diff --git a/.agents/skills/agents-md/SKILL.md b/.agents/skills/agents-md/SKILL.md new file mode 100644 index 00000000..0a49fbcc --- /dev/null +++ b/.agents/skills/agents-md/SKILL.md @@ -0,0 +1,53 @@ +--- +name: agents-md +description: Discipline for writing and updating AGENTS.md (or CLAUDE.md) so it holds only what an agent cannot infer - policy, gotchas, and pointers - and stays under about 40 lines. Use instead of /init when creating, editing, or adapting one. +--- + +# AGENTS.md + +`AGENTS.md` is loaded on every task, so every line is a cost paid everywhere. +Measured on real repositories (arXiv 2602.11988): developer-written files raised resolve rate about 4% and cost about 20%; LLM-generated files lowered resolve rate; repository overviews did not help. +What helped agents was locating the right place to change, not writing better patches. + +## The test + +A line earns its place when both hold: + +> Would it cost more than one cheap feedback round to discover and fix? +> Does no skill or always-read file already state it? + +A lint or formatting miss costs one run of CI or of the repository's check command and is not worth a line; a testsuite written against the library instead of the CLI is a pile to redo, and is. +Put positively: there is no way the agent learns this unless we write it. + +## What belongs, in order of value + +- A pointer to a non-standard layout: "Integration tests are centralized in `testsuite`; see `testsuite/AGENTS.md`." +- The default feedback loop when it is not the ecosystem's default: "`cargo xtask check-uncommitted` verifies local modifications." +- A policy with its reason: "Use `#[expect(lint, reason = "...")]` so the suppression explains itself and expires when it stops being needed." +- Repository rules no tool enforces: intent-file handling, writing conventions for committed text, comment markers such as `INVARIANT:` and `UNTESTED:`. +- The one fact a shared skill leaves to the repository, such as the issue-reference footer format, when it fits in a line; otherwise a repo-local skill. +- One opener saying where skills live, for agents that do not discover them. + +What `/init` generates is the blacklist: product overview, directory map, command lists, "follow standard conventions", inventories of packages or scopes, and lossy summaries of `STYLE.md`. +Examples without the rule behind them are the costliest lines; agents copy the example, mistakes included. + +## Division of labour + +```text +AGENTS.md always loaded policy, gotchas, pointers +skill loaded on match rules for one task; may load the skills it builds on +STYLE.md loaded by a reviewer skill when a rule's intent is unclear +``` + +The root file enters every context, so about 40 lines is the rule of thumb; a nested file loads for fewer tasks and may run longer; past the mark, flag it for the human rather than cut a gotcha to fit. +Load each file at the latest point where it pays for itself; an always-loaded file pointing at a 700-line guide is a cost on every task, while a skill pointing at the one it depends on keeps both small. +A directory with its own policy gets a nested `AGENTS.md` and one pointer from the root. +Every `AGENTS.md` gets a sibling `CLAUDE.md` symlink to it, because Claude Code reads only `CLAUDE.md`. + +## Keeping it true + +- Verify every surviving sentence against the tree and the remote default branch; `/init` files go stale within weeks. +- When a line documents a workaround, fix the code instead; the file is a list of smells not yet fixed. +- When adapting another repository's file, report deviations with reasons instead of copying: a line true there may be false here. + +`references/before-after.md` holds one 151-line file reduced to 27, with the reason each block was cut or kept and the maintainer's verbatim pushbacks; read it when unsure what counts as discoverable. diff --git a/.agents/skills/agents-md/references/before-after.md b/.agents/skills/agents-md/references/before-after.md new file mode 100644 index 00000000..33e4b134 --- /dev/null +++ b/.agents/skills/agents-md/references/before-after.md @@ -0,0 +1,65 @@ +# Before and after: Devolutions Gateway `AGENTS.md` + +`git diff 6b906d7d b6c98d77 -- AGENTS.md` in Devolutions/devolutions-gateway, PR #2091. +151 lines became 27. +IronRDP's equivalent is Devolutions/IronRDP#2131 (155 to 38). +The result is `git show b6c98d77:AGENTS.md`: five headings (intent files, writing for the repository, Rust, untested-property comments, Git) under a one-line skills opener. + +## What was cut, and why it was cuttable + +| Block in the old file | Verdict | Evidence | +|---|---|---| +| `Issue: #456 (GitHub issue)` as a valid footer | Wrong | `cliff.toml` renders every `Issue:` value as a Jira URL; four forms of the mistake in the last 300 commits | +| Scopes `dgw`, `agent`, `jetsocat`, `webapp`; "library scopes `*deps*`, `*openapi*`, `*npm*`" | Stale | `installer` and `agent-installer` missing; `cliff.toml` suppresses only exact `chore(deps)`/`build(deps)`; the `commit-scope` skill already had the right table | +| Branch naming (`DGW-338-...`, `fix/123-description`) | Never a convention | 2 of the last 60 merged PR branches carried a Jira key | +| ~70 lines of PR title/body/footer/examples | Duplicated | `commit-and-pr-conventions` and `commit-scope` skills | +| ~55 lines of Rust style (errors, logging, comments, invariants, size constants) | Duplicated, lossy | a summary of IronRDP `STYLE.md`, vendored by the same PR and served by the `rust-style` skill | +| `## Project Overview` workspace listing | Discoverable | `Cargo.toml`, `webapp/pnpm-workspace.yaml` | +| `## Additional Resources` (`Main README: README.md`, `Cookbook: docs/COOKBOOK.md`) | Discoverable | one `ls` | +| TypeScript and PowerShell command lists | Discoverable | `webapp/package.json` scripts, `powershell/*.ps1` | +| `Tests go in testsuite/tests/, helpers in testsuite/src/`; `Consider proptest, rstest` | Discoverable | the tree; replaced by one pointer to `testsuite/AGENTS.md` | +| `Follow standard .NET conventions.` / `Follow standard PowerShell conventions.` | Content-free | | +| `cargo clippy --workspace --tests -- -D warnings`, `cargo test --workspace` | Plain command | CI annotations print the command to run | + +## What was kept or added, and the test it passed + +| Line | Why it earns its place | +|---|---| +| Nightly rustfmt | Kept at the time as a one-round gotcha; under the feedback-round test it is borderline, since CI reports the fix in one round, and the maintainer is inclined to drop it. IronRDP dropped it because it formats with stable: verify before copying | +| `#[expect(..., reason = "...")]` with its reason | Policy; the old text said `THINK before suppressing` without the why | +| Pointer to `testsuite/AGENTS.md` | Non-standard Cargo layout; details live in the nested file (26 lines: run the binary, dependencies are a design signal, fewer stronger tests, timing tests run serially) | +| `Issue: DGW-123` | The shared commit skill defers footer format to the repository; the one-line form stays here and the full rules live in the repo-local `commit-footer` skill the line points to | +| Intent files, writing for the repository, untested-property comments | Policy no agent infers and no skill holds; taken from IronVNC and picky-rs | +| Skills opener | For agents that do not auto-discover `.agents/skills/` | + +## Maintainer pushbacks, verbatim + +From Gateway, IronVNC, and sspi-rs `AGENTS.md` reviews. + +- "it contains redundant information that is better derived by each agents on demand" +- "Remove everything that can be explored naturally (e.g.: layout section)" +- "I would not include any 'obvious' cargo command, it may be worth keeping the line about using nightly for formatting, and to think before silenting lints." +- "I don't think we even need the introduction about what is Devolutions Gateway" +- "I think we can refer to rust-style in the top-level AGENTS.md, but I would keep rust-style-reviewer mentioned only in the code-review skill." +- "Don't we risk to have the agent pull off the full file? Is it better to refer to the STYLE.md file only in the reviewer skill?" +- "Also verify the Git section does not duplicate the other matching skills." +- "The best place for that may be in the matching skill instead of AGENTS.md" +- "The property is that we run them serially, it may be good to explain that" (a timing-tests section had described fixed delays instead of the property) +- "I think this is not necessary, agents already do it without instructions" ([sspi-rs #763](https://github.com/Devolutions/sspi-rs/pull/763#discussion_r4195986262)) +- "Since this can be derived easily from the workspace Cargo.toml, I would tend to remove that" ([sspi-rs #763](https://github.com/Devolutions/sspi-rs/pull/763#discussion_r4196024203)) +- "Do you think of a specific anti-pattern?" and "I need some more meat on this to understand what that is concretely. What are we asking the agent to do?" ([sspi-rs #763](https://github.com/Devolutions/sspi-rs/pull/763#discussion_r4196018926)) +- "I think this is better suited as a review-style skill, rather than the top-level AGENTS.md" ([sspi-rs #763](https://github.com/Devolutions/sspi-rs/pull/763#discussion_r4195993368)) +- "It's likely enough to suggest `cargo xtask check wasm`, and remove the note about native threads, filesystem, etc. Even if the agent would use such APIs, it would be blocked by `cargo xtask check wasm`." ([IronVNC #1544](https://github.com/Devolutions/IronVNC/pull/1544#discussion_r4024093374)) +- "I would remove the heading of all files where it's not really necessary. This is read by agents, and it's found in a folder called 'ironvnc-fuzzing'." ([IronVNC #1542](https://github.com/Devolutions/IronVNC/pull/1542#discussion_r4013920553)) +- The positive form: "Perfect, there is no way the agent can learn about this if we don't write it in the AGENTS.md" ([sspi-rs #763](https://github.com/Devolutions/sspi-rs/pull/763#discussion_r4196027485)) + +## Resulting reference graph + +```text +AGENTS.md ──► rust-style skill (no pointer onward) +code-review skill ──► rust-style-reviewer skill ──► STYLE.md (only when a rule's intent is unclear) +AGENTS.md ──► testsuite/AGENTS.md +AGENTS.md ──► commit-footer skill +``` + +Each file is loaded at the latest point where it pays for itself. diff --git a/.agents/skills/blind-testsuite/SKILL.md b/.agents/skills/blind-testsuite/SKILL.md new file mode 100644 index 00000000..06494b8f --- /dev/null +++ b/.agents/skills/blind-testsuite/SKILL.md @@ -0,0 +1,35 @@ +--- +name: blind-testsuite +description: Write or extend a conformance suite from a CONTRACT*.md and published sources only, by an author who never sees an implementation, kept read-only while implementations are built against it. Use when a contract has or will have several implementations, or a wrong expected value would be invisible to the implementer, as in cryptography, codecs, and wire protocols. +--- + +# Blind test suite + +The suite is independent evidence derived from the contract, not the definition of conformance, so a failing test says the implementation or the test is wrong, never the contract. +`test-writer` decides each test's shape and `test-oracle-reviewer` what counts as an independent value; this skill is the isolation around them. + +## What the author sees + +The contract file, the interface it is written against (the contract crate or module, its types and documentation), and published sources: specifications, vendored vectors, captures. +Not an implementation, an earlier implementation, or the pull request that produced the contract, because each carries a reading of the contract the suite would then confirm rather than test. +When an implementation exists, the author is a fresh context in a worktree at a commit without it; a do-not-read list supplements that and does not replace it, since a context that has seen the implementation cannot unsee it. + +## Expected values + +From a published source cited in the test, a property the contract states, or a derivation whose method the contract or cited standard fixes, recorded beside the vector so a reviewer can redo it; never from a computation only an implementation of the contract could perform, because checking such a value means implementing the contract again. +A derived vector accounts for every coupled field the change touches - length, checksum, signature - or its outcome is not what the derivation claims. +Where no published answer exists, compare outputs across implementations; a differential check never stands alone, because a bug shared by both passes it. +A hand-written negative input differs from an independently valid control in the targeted dimension only, so the test can fail only for the reason it names; a rule that names a rejected input gets one, since such a rule tested only by acceptance is met by accepting everything. +Vendored sources keep origin, version, and checksum recorded, are taken whole or cut at their own section boundaries, and are never edited, because an edited or sliced vector loses the provenance that makes it an oracle. +Keep a coverage record beside the suite: which test checks each clause, which clauses are verified by review only, and the boundaries deliberately left unchecked, so silence is not taken for coverage. + +## Review and merge + +The reviewer runs `test-reviewer` and checks the contract against the coverage record both ways: every clause has a test or a recorded gap, every test cites its clause. +Write the suite before the implementation; merge it once one implementation runs it green, so a test no conforming implementation can pass is found before it binds every later one. + +## Read-only during implementation + +An implementation pull request never edits the suite or the contract. +A test that looks wrong, or a clause with no test, is escalated with the vector or clause, expected and observed values, and the cited source. +The fix goes to the pull request that owns the suite - the open one through its lead, or a new one once it has merged - by a fresh author with the same inputs, who receives the decision and not the implementation's output, so the suite stays blind. diff --git a/.agents/skills/commit-and-pr-conventions/SKILL.md b/.agents/skills/commit-and-pr-conventions/SKILL.md index 87daf4b6..b1c6c471 100644 --- a/.agents/skills/commit-and-pr-conventions/SKILL.md +++ b/.agents/skills/commit-and-pr-conventions/SKILL.md @@ -38,5 +38,6 @@ Issue: PROJECT-123 - Do not manually wrap the PR body; GitHub handles display wrapping. - Keep permanent history concise: record motivation, material behavior, and side effects. - Put experiments, alternatives, extensive validation, and implementation chronology in follow-up comments. +- Post a follow-up comment with the lines changed, split at least into production code, tests, and other files, so a reviewer sees where the change's weight lies before reading it; how to count and present them is yours, and refresh it when the diff changes materially. - Update the title and initial body as the implementation evolves. - Before opening, updating, or merging, read `title + blank line + body` as one commit message and correct any mismatch. diff --git a/.agents/skills/contract-authoring/SKILL.md b/.agents/skills/contract-authoring/SKILL.md new file mode 100644 index 00000000..70795a29 --- /dev/null +++ b/.agents/skills/contract-authoring/SKILL.md @@ -0,0 +1,56 @@ +--- +name: contract-authoring +description: Author or extend a normative CONTRACT*.md - an exact, testable specification of an interface, protocol, or provider boundary that independent implementations and a conformance suite derive from. Use when designing a spec, contract, protocol document, or provider API that several components or backends must agree on. +--- + +# Contract authoring + +A contract defines a surface that two independently built parties must agree on: a backend author implements from it alone, and the conformance suite derives its expectations from it and the standards it cites, never from an implementation. +It names the parties in scope without schedule ("e.g.: Product A, Product B", not "A now, B later"), since a release stage is not a rule a party agrees to, and a statement belongs only if one of them must agree to it. +Of a party's own realisation (`intent-files`), only the parts a named second party depends on enter. +`INTENT.md` beside it explains the choices; the contract states the rules. +Name the file `CONTRACT..md` when the repository may hold more than one contract, and cite it everywhere as `CONTRACT..md §`, because the file name is the one citation that is unique, searchable, and readable without a lookup. + +## Process + +1. **Scout before writing.** + Inventory every call site the contract will serve, with `file:line`. + Read each reference implementation or host at a pinned version and record, per operation, whether it is native, argument mapping, workaround, or unsupported, with evidence. + Host conventions (naming case, pagination, proxies that rewrite paths) become rules; a reference you did not read becomes a false universal. +2. **Settle the design by interview**, one frontier of decisions per round as in `grill-intent`; the human decides, and the agent may draft the normative wording for each decision, since a contract is agent-written under human approval. + Number every clarification and log who decided it. +3. **Draft, then critique with two lenses**: overkill and gaps. + Run every state-changing operation through `references/critique-lenses.md`. +4. **Freeze** once the design is approved. + Afterwards it changes only for implementation defects, in its own PR ahead of the code that needs it, so the change is reviewed as a rule and not excused by the code it unblocks. + Implementers never edit the contract or the test suite to make code pass; they escalate with evidence from the cited source. + +## Content rules + +- Per operation: the valid domain, the must-support set, the exact error for each class of invalid input from a closed error set, and the precedence of outcomes. + Phrase precedence as which outcome wins, not as an evaluation order, since only the outcome is observable and an order binds a backend's internal checks. +- Implementation-defined cases are listed with their exact allowed outcome set, never with examples, and are excluded from differential tests. +- For every stored field: who initialises it, which operations update it, and which must not. +- Every list exists once; a second copy may drift. +- A `Definitions` section with one entry per term the parties must share, in the glossary shape from `intent-files`; one encoding per concept, and the same word for the same thing throughout. +- The contract is the single source for the API it specifies; when it covers a code API, public items cite their section and a check keeps them in step. +- Each operation and normative section has an anchor as `intent-files` Anchors defines, because the conformance suite cites every rule and the code and the intent cite the operations they realise. +- Detail beyond the intent is the contract's job, but when a contract rule needs a design decision the intent does not take, the human records that decision in `INTENT.md` first; a design decision made only in the contract is one nobody made. +- A contract has no versions and no deferral markers: it describes the whole surface as it must be, and the subset realised today is a deferral in the co-located intent file, extended by editing; a version negotiated on the wire is content. +- An example is not a rule: state what is free (order, case, whitespace) and add vectors that vary it, or a conforming library will reproduce none of them. +- Put the exception where the rule is: "terminal, except during pending-key recovery". +- Strict where it matters, extensible elsewhere: optional fields older receivers ignore, but authentication data stays strict, since an ignored field in authentication data is an unverified input. +- A derivable operation stays out unless a written exception (performance, validated-module boundary, a security property) sits next to it, since each one widens what every backend must implement and test for a result the primitives already give. +- Leave out one library's behaviour and numbers, admin and UI surfaces, mock and test-harness control surfaces (those are the intent of the component that implements them), implementer choices, and process. +- A rationale must be as true as its rule, since a reader who finds the cause false drops the rule with it; measure before citing a cause. + +## Testability + +Write each rule with one condition and one response ("when …", "while …", "if … then …") and one meaning per verb: declarative present for a requirement, `should` a recommendation, `may` a permission, `can` a capability. +Each rule maps to a conformance case, worded as an observable with a bound: "encode writes exactly `size()` bytes", never "handles" or "supports", which name no outcome a test could check and so let divergent implementations all comply. +"The backend supports RSA-PSS" is met by signing, by returning `Unsupported`, or by producing bytes nobody verifies; "signing with RSA-PSS returns a signature that verifies under the public key, or `Unsupported` before any key material is read" is one outcome and one test. +The contract's rules define conformance, and it has no conformance section, because defining conformance as passing a suite makes the evidence the rule, so a party that follows the contract stops conforming when the suite has a bug. +Test vectors and suites are evidence for the rules, so their role, their pass conditions, the mapping of rules to tests, the review-only claims, and the known gaps belong to the suite and the intent that governs it, never to the contract. + +Beyond that, shape is free: order sections so a reader finds all of an operation's rules in one place. +`references/patterns.md` lists what review kept and what it fixed in two agent-written contracts; read it when revising one or checking a draft. diff --git a/.agents/skills/contract-authoring/references/critique-lenses.md b/.agents/skills/contract-authoring/references/critique-lenses.md new file mode 100644 index 00000000..f7321b76 --- /dev/null +++ b/.agents/skills/contract-authoring/references/critique-lenses.md @@ -0,0 +1,30 @@ +# Two-lens critique + +Run both lenses on a complete draft before freezing, ideally as an independent reviewer that has not seen the design conversation. +Report with severities and section references; the author adjudicates each finding in writing. + +## Lens A: overkill + +For each mechanism ask what breaks without it: two modes or encodings that could be one, a tombstone or knob serving a case nobody has, a configurable value that could be fixed, an admin or debug surface only one product or only tests need. +Keep what has a named failure; cut the rest, or move it to the intent or document of the party that owns it when it is that party's own realisation. + +## Lens B: gaps + +Put every state-changing operation through each question. + +1. **Lost response.** The request succeeded but the reply never arrived; can the client retry with no side effects and recover the result? +2. **Deleted in between.** The object the retry references was deleted or exhausted meanwhile; does the retry still recover, and what stops the deleted thing from doing more than that? +3. **Concurrency.** Two identical requests race; does exactly one win, is the loser handed the winner's result, and is uniqueness enforced by the store rather than by a check? +4. **Expiry mid-flow.** A token, certificate, or grace window expires between steps; does the flow strand a recoverable state? +5. **Trusting the client's declaration.** Does the verifier accept the client's own list of what it covered, or require exactly what the profile mandates? +6. **Make-before-break.** Does a rotation or renewal close the old path before the new one is proven to work? +7. **Order versus precedence.** Does the text fix an evaluation order where it only needs to fix which outcome wins, or leave two outcomes equally valid for one input? +8. **Bounds.** Are sizes, counts, and nesting capped, and does an aggregate cap agree with the per-item caps? +9. **Malformed error bodies.** What does the client do when the error response itself is wrong? +10. **Second copy.** Is any list, mapping, or encoding defined in two places? + +## For each new rule + +- Was this written to fit one implementation? +- Would a reader with only this document and the cited standards produce the same bytes? +- What test fails if an implementation ignores it? diff --git a/.agents/skills/contract-authoring/references/patterns.md b/.agents/skills/contract-authoring/references/patterns.md new file mode 100644 index 00000000..1bee3ef5 --- /dev/null +++ b/.agents/skills/contract-authoring/references/patterns.md @@ -0,0 +1,25 @@ +# Patterns from two agent-written contracts + +`docs/agent-identity/CONTRACT.agent-identity.md` (Devolutions/devolutions-gateway, PR #2000) and `picky-crypto/CONTRACT.md` (Devolutions/picky-rs) were written by agents under maintainer review. +They are not shape references; use them to recognise what review kept and what it had to fix. + +## Kept by review + +- General rules that apply to every operation, stated once at the top with "these rules are not repeated", so per-operation sections stay short. +- Precedence as which outcome wins, not an evaluation order: "a bounds error always wins; a capacity limit may win over a structure error; this fixes which outcome wins, not the order among a backend's checks". +- Implementation-defined outcomes as closed sets per input: "loading returns `InvalidKey` or succeeds; signing returns `InvalidKey` or a signature that verifies under (n, e)". +- Stored-field ownership as one sentence naming the operations: "renewal, confirm and check-in update `last_seen_at`; an idempotent enrollment replay and stream closure do not". +- The error section as the only list of codes, mappings, and retry rules. +- Exceptions inline with the rule they relax. + +## Fixed by review, so look for them + +- Claims that overreached and were narrowed: "prevents oracles" became "does not reveal which check failed"; "cannot be compared" became "cannot be compared directly". +- One library's behaviour as a general rule: "p = q" for an inconsistent RSA key. +- A number and a rationale tied to one backend, with the cause never measured. +- A second copy of a list: the deletion list missed `device_revoked`. +- An example standing in for a rule: signature parameter order was shown, not stated, so a conforming library reproduced no vector. +- Two encodings for one concept: hex and base64url for the same hash. +- Lost-response, deleted-in-between, concurrency, and expiry-mid-flow paths unspecified until a critique asked. +- A verifier accepting the client's own list of covered components. +- A product's admin API and the mock's control surface inside the contract; both are product or test surface, and each belongs in the intent of the component that implements it. diff --git a/.agents/skills/contract-review/SKILL.md b/.agents/skills/contract-review/SKILL.md new file mode 100644 index 00000000..bd449c10 --- /dev/null +++ b/.agents/skills/contract-review/SKILL.md @@ -0,0 +1,52 @@ +--- +name: contract-review +description: Review a normative CONTRACT*.md for overreach, unspecified failure paths, duplicated lists, examples standing in for rules, one-implementation bias, and untestable wording; returns findings with proposed rewordings, never edits. Use when asked to review, critique, tighten, or prepare a rewrite of a contract or protocol specification. +--- + +# Contract review + +A contract is implemented and tested by parties who read only it, so every ambiguity becomes a divergence between implementations. + +## Gather + +State the revision reviewed. +Collect the contract, the `INTENT.md` it serves, the conformance suite or test vectors derived from it, and each implementation. +Run every state-changing operation through the gap questions in `contract-authoring/references/critique-lenses.md`; `contract-authoring/references/patterns.md` lists what earlier reviews had to fix. + +## Lenses + +Apply each to every normative sentence and quote it, so the author finds it after edits. + +- **Party**: which second party in scope must agree to this sentence? + None means it is one party's own realisation (config, storage, CLI) and belongs in that party's intent file. +- **Intent contradiction**: the contract says what its `INTENT.md` excludes, or excludes what the intent commits to; detail beyond the intent is expected, disagreement is not, and which side is wrong is the human's call. +- **Stage talk**: "V1", "initially", "will be", "for now", or a party named with a stage ("A now, B later" for "e.g.: A, B"), since a release stage is not a rule a party agrees to; the contract states the whole surface, and what is realised today is the co-located intent file's deferral. +- **Overreach**: a claim stronger than the mechanism guarantees ("prevents oracles" for "does not say which check failed"); propose the narrowed form. +- **Open set**: an error, outcome, or input class not drawn from a closed, defined set; "bad input" is not a classification. +- **Order for precedence**: an evaluation order where only which-outcome-wins matters, or two outcomes equally valid for one input. +- **Unowned field**: a stored field with no statement of who initialises it and which operations update it or must not. +- **Second copy**: a list, mapping, or encoding defined twice; keep one and point. +- **Anchor**: an operation or normative section without an anchor, a published anchor renamed, or an anchor removed while citations still name it, since the suite, the code, and the intent cite sections by anchor (`intent-files` Anchors). + Propose the id or restore the old one; for a removal, list the citers a search for the id finds. +- **Example as rule**: a transcript or template whose free aspects (order, case, whitespace) are not stated; propose the statement and vectors that vary them. +- **One-implementation bias**: a library's behaviour, a number, or a rationale true for one backend; ask whether it was written to fit one implementation. +- **Unmeasured cause**: a rationale citing a mechanism nobody measured. +- **Untestable**: "handles", "supports", "properly", a condition without a stated outcome, or two outcomes in one sentence; propose an observable with a bound and the conformance case that checks it. +- **Scope creep**: admin, UI, mock control, implementer choices, a conformance section or a "must pass the vectors" rule (test vectors and suites are evidence for the rules, not rules), an acceptance rule for the work such as "passes the suite before merge", or any section that neither constrains a party in scope nor is exercised by a conformance test; propose the owning component's intent file, the suite's coverage record, the process rules, or deletion. +- **Intent gap**: a contract rule that takes a design decision `INTENT.md` does not, as opposed to adding detail; the human records the decision in the intent first, since a decision made only in the contract is one nobody made. + +## Report + +Order by consequence: party, intent contradiction, and gaps, then overreach, open sets, second copies, and anchors, then wording. + +```md +### . — §
+ +> + + + +**Proposed:** , or move to
, or delete. +``` + +Close with what is strong as written, the questions only the human can settle, and a verdict: freeze, freeze after corrections, or rewrite. diff --git a/.agents/skills/contribute/SKILL.md b/.agents/skills/contribute/SKILL.md new file mode 100644 index 00000000..1b409e37 --- /dev/null +++ b/.agents/skills/contribute/SKILL.md @@ -0,0 +1,36 @@ +--- +name: contribute +description: Pick the delivery flow by scale - a test-first fix in place, one led pull request, or an orchestrated multi-pull-request effort - and where the intent, contract, test, and review skills enter. Use at the start of any non-trivial change, or when asked how to approach or stage work. +--- + +# Contribute + +Every flow is the same loop at a different scale: decide what must hold, write the check that fails without it, make it pass, and have someone who did not write it review. + +## Pick the smallest flow that fits + +| Flow | When | Run | +|---|---|---| +| Fix | One behaviour at one seam; every decision it needs is already in the intent, the contract, or the bug report | `tdd` here, then `code-review` | +| Change | One pull request; may raise a design decision or need more than one pair of hands | `grill-intent` for any decision none of those states, then `lead` | +| Effort | Several pull requests, a second party that must agree on an interface, or a suite written blind | `orchestrate-effort` | + +Promote when a signal appears rather than starting big, because each tier adds sessions and context to keep in step. +A decision neither the intent, the contract, nor the report states promotes to Change: stop and run `grill-intent` rather than settling it in a test or code, since the first place a decision lands becomes the text everything else is bent to. +A second party or a `CONTRACT.md` change promotes to Effort: the contract is its own pull request before the code that realises it. + +## Where the other skills enter + +- `intent-files`: read every applicable intent file before the first edit, in every flow. +- `contract-authoring` and `contract-review`: when two independently built parties must agree; the contract precedes its first implementation. +- `blind-testsuite`: when a wrong expected value would be invisible to the implementer, as in cryptography, codecs, and wire protocols; written from the contract before any implementation. +- `test-writer` for every test, `test-reviewer` and `test-oracle-reviewer` through `code-review`, `dependency-reviewer` through `code-review` when a manifest or lockfile changes, and `dependency-pruner` then `test-pruner` on the diff once the code and tests are complete and before the last review. +- `intent-drift-audit`: once a realisation is claimed complete, in the lead's final check or the reviewer's pass, never mid-implementation. +- `variants`: when a named design choice needs implementation evidence rather than a paper answer. +- `review-feedback-loop`: once a pull request has findings. + +## Wrong-flow signals + +- An expected value you cannot justify independently of the code under test: the oracle, perhaps the seam, is wrong; see `test-oracle-reviewer`. +- An intent file, `CONTRACT.md`, or blind suite this pull request does not own edited inside it: that change belongs to the pull request that owns the text. +- More open pull requests than you can verify one by one: stop opening new ones. diff --git a/.agents/skills/dependency-pruner/SKILL.md b/.agents/skills/dependency-pruner/SKILL.md new file mode 100644 index 00000000..cfa1cbf0 --- /dev/null +++ b/.agents/skills/dependency-pruner/SKILL.md @@ -0,0 +1,30 @@ +--- +name: dependency-pruner +description: Audit the dependencies a change added or widened before its pull request opens, keep each only where it is load-bearing, and replace the rest with a few lines, a lighter package, or one already in the tree. Use at the end of a change, as the last worker task before the pull request opens, or when asked to trim or justify a package's dependencies. +--- + +# Dependency pruner + +A dependency is a maintenance contract: every one costs build time, audit surface, and a future upgrade, so each must be load-bearing, which means removing it would cost more than it saves. +Run once per change, on the diff, after the code is complete, because a package added mid-way is often not needed by the end. + +## Inventory + +List every direct dependency, feature, and version bump the manifest diff adds or widens, then every package the lockfile diff adds and the direct dependency or feature that pulls it in, since a feature flag or a transitive addition expands the build as much as a new direct entry. +For each direct one, find the code that uses it and write down what it does there and what removing it would take. + +## Decide + +- Replaceable in a few straightforward, testable lines of safe code: replace it, because the package costs every upgrade and the lines cost one review; except where the lines would reimplement security-sensitive or standards-defined behaviour - parsing, cryptography, encodings - which is where a short implementation is wrong in ways no test of yours catches. +- Replaceable only with `unsafe`: keep it, since `zerocopy`, `bytemuck`, and their kind are audited where a hand-written transmute is not. +- A proc-macro in a foundational package - one kept to few dependencies because many others build on it - goes unless the feature cannot exist without it, because a macro adds a host-side build and the common derive stacks bring `syn` into every dependent's build graph; an error type there is `Display` and `Error` written by hand, not `thiserror`. +- Already in the tree, directly or transitively: reuse it rather than add a second package for the same job. +- Needed but heavy: look for a minimal alternative that does only the needed part. +- Two candidates remain: take the one with the stronger maintenance evidence - issues and pull requests answered, releases when fixes were needed, more than one active maintainer - since the one that stops being maintained becomes your code. +- A dependency the applicable intent file allows or forbids by name follows the intent. + +## Change + +Remove what the decisions say to remove, then inspect the resolved graph and run the repository's dependency policy checks (for Cargo, `cargo tree -d` for duplicate versions and the feature graph for enabled edges), because a removal changes resolution and feature unification for packages you did not touch. +Commit the pruning apart from the feature work, so its review is about dependencies and nothing else. +Report every kept dependency with what it does and why replacing it costs more, every removed one with what replaced it, and the net change in direct and transitive counts. diff --git a/.agents/skills/dependency-reviewer/SKILL.md b/.agents/skills/dependency-reviewer/SKILL.md new file mode 100644 index 00000000..89345c15 --- /dev/null +++ b/.agents/skills/dependency-reviewer/SKILL.md @@ -0,0 +1,17 @@ +--- +name: dependency-reviewer +description: Review the dependencies a diff or pull request adds or widens against the `dependency-pruner` rules - load-bearing, no proc-macro in a foundational package, plain code over a package unless unsafe, minimal and maintained alternatives - and flag what is probably not necessary. Use when a diff changes a manifest or lockfile, or when asked whether a dependency belongs. +--- + +# Dependency reviewer + +Invoke `dependency-pruner` and use its decision rules as review criteria without running its procedure; this skill judges the manifest and lockfile changes and does not edit. +Review only dependencies the diff adds or widens, features and transitive additions included, unless the user asks for a whole-package audit. + +For each direct addition, find the code that uses it and ask what removing it would take; for a transitive one, find the direct dependency or feature that pulls it in. +A few straightforward, testable lines of safe code is a finding, with the lines sketched, unless they would reimplement security-sensitive or standards-defined behaviour; a replacement needing `unsafe` is not. +A proc-macro added to a foundational package is a finding unless the pull request names the feature that cannot exist without it and the diff bears that out. +A package that duplicates one already in the tree, directly or transitively, is a finding naming the existing one. +A heavy package used for one small part is a finding naming the minimal alternative when one exists; absent one, say so rather than guess. +A dependency whose pruner report entry survives these questions is not a finding; one with no entry is, because the author's verification is the evidence that it is load-bearing. +Report each finding with the manifest line or the lockfile entry and its parent edge, what the dependency does in the diff, and the replacement or the evidence that would justify keeping it; return no findings when every addition is load-bearing. diff --git a/.agents/skills/docs-writer/SKILL.md b/.agents/skills/docs-writer/SKILL.md index 4ad9c696..1a8d2590 100644 --- a/.agents/skills/docs-writer/SKILL.md +++ b/.agents/skills/docs-writer/SKILL.md @@ -16,9 +16,7 @@ Keep examples minimal, realistic, and responsible for unique information. Place warnings and constraints beside the step they affect. Preserve established terminology and verify technical claims against the implementation. -Write the requested documentation directly. -Prefer no net word-count growth when restructuring existing documentation without adding requested information. -New structure should replace or consolidate prose; allow small justified growth when it materially improves navigation without duplication. -Before finishing, remove repetition and confirm that a reader can find the next action without reconstructing it from implementation details. -Don't add documentation comments for code that can be easily understandable. -Don't add implementation details in the public API documentation - keep such information private (e.g. inside the function directly). \ No newline at end of file +When restructuring, new structure replaces or consolidates existing prose instead of adding to it. +Write a comment or doc comment only for non-obvious behavior, invariants, or rationale, never to restate the code. +Public API docs state caller-visible behavior and guarantees, because callers treat whatever they say as a contract; put implementation details in comments inside the item. +Before finishing, confirm that a reader can find the next action without reconstructing it from implementation details. \ No newline at end of file diff --git a/.agents/skills/grill-intent/SKILL.md b/.agents/skills/grill-intent/SKILL.md new file mode 100644 index 00000000..f15560f5 --- /dev/null +++ b/.agents/skills/grill-intent/SKILL.md @@ -0,0 +1,73 @@ +--- +name: grill-intent +description: Round-based interview in which the human writes each design commitment in their own words and the agent supplies context, consequences, and verification, then drafts INTENT.md or *.intent.md with provenance. Use when the user wants to think through or bootstrap intent files rather than have them written from code or a brief. +--- + +# Grill for intent + +Intent is the human's (`intent-files`): the human writes each commitment, and the agent's part is to find the decisions, give the context each needs, verify what the human decides, and tidy the wording to its conventions. + +## The tree + +Root questions, from which everything else branches; settle vocabulary first, because exact nouns make most commitments one sentence: + +- **Vocabulary**: which terms does this unit coin, what is each, which synonyms are avoided, and what rule does each definition carry? +- **Boundary**: which unit, and what is deliberately outside it? +- **Role**: why does it exist; what is lost if it is folded into a neighbour? +- **Must remain true**: what would make the software wrong even with green tests? +- **Rejected alternatives**: where was one way picked over a reasonable other, and why does the other stay excluded? +- **Non-goals**: what is unsupported or was cut? +- **Properties**: security, compatibility, concurrency and lifecycle, dependency boundaries, extension points. +- **Future**: which directions must not be precluded? +- **Second party**: which other party must agree to anything here, so that a `CONTRACT.md` is warranted, and which interfaces have no such party and stay intent? + +## Rounds + +The frontier is every question whose prerequisites are settled. +When code exists, open with a short brief as its own turn, before any question: what the component is, what it does today, its backends, callers, and dependencies, in plain language and in the human's vocabulary. +End the brief by inviting the human to ask for more or to confirm it; expand where asked, and ask the first round only after they confirm, since a short brief they confirm still catches the places where their memory and the code disagree. +Ask the whole frontier in one round, numbered, with the context the round needs as annexes below the questions: each annex holds the facts for one question or one shared topic, cited with `path:line`, and a question refers to its annex rather than restating the code. +Each question gives the human what they need to decide: the fact it turns on, what the options are, and how each could go wrong later; it does not recommend an answer, because a recommended answer the human accepts is the agent's design under the human's name. +Plain language, checkably: a question names behaviour, not identifiers; identifiers and `path:line` live in the annex; the skill's own vocabulary ("frontier", "closed set") never reaches the human. + +```text +❓ **Q1** - ****: <what is at stake, in one or two sentences> +<Today: the fact this question turns on; details in annex A.> +<Options as short labels, each with how it could go wrong.> +<Write your commitment in your own words, or say "no commitment here".> + +--- + +**Annex A** - <topic>: <facts with path:line; alternatives.> +``` + +Wait for answers, recompute the frontier, repeat. +Look up dependencies, features, callers, and siblings before asking. +Offer options only when they are a closed set the facts define (the variants that exist, yes or no on what the code does today, the list it already uses, two readings of the human's own sentence), as labels rather than finished sentences so that the human writes the sentence, and always include "no commitment here". +Alternatives the human may not know are information, not options: mention them in the annex with what each is good for and where it fails, so the human can widen the choice, and then let them write the commitment. +When the human asks for a recommendation, show what their earlier answers already settle and assemble those words into a candidate sentence for them to accept or rewrite; when they delegate outright, write it and record it as agent wording the human adopted. +When an answer arrives, verify it against the code and the other answers; if it is false today, contradicts another commitment, or has a consequence the human may not have seen, say so with the evidence and ask them to confirm or rewrite before recording it. +Record the human's sentence; tidy it to the conventions only, keeping their words where they are exact, and do not re-present tidied sentences each round, since the final list is where the human confirms them. +Keep a ledger of alternatives: ones the human voiced, options they hesitated between, and any you hold strongly, each with its reason, so that the rejected ones reach the intent file as rationale and the unsettled ones can become an experiment. + +## Grill like a reviewer + +- An absolute: ask which boundary it protects and what falsifies it; show the sibling, feature flag, or build that would, and let the human scope it. +- Wishing or future tense: ask whether the behaviour exists; if so, ask for the sentence as a requirement, otherwise for the deferral marker. +- A tentative want ("I'd like", "I have in mind", "may"): ask whether it is a requirement, a permission, or a preference, and for the sentence that says which. +- A rule without a reason: ask what alternative it excludes and why. +- A list of variants: ask whether the rule is the list or a property each variant satisfies. +- A fuzzy or overloaded word ("account", "session"): show where the code and the conversation use it differently, and ask for the canonical term and the avoided ones. +- "Requires a decision", "run X after Y", anything about contributors: process; note it for `AGENTS.md` and ask for the software property behind it. +- Exact detail (fields, limits, status codes): note it for a `CONTRACT.md` if a second party must agree to it, and return to the decision behind it. +- A decision about a sibling component: note it with its destination file for the handoff, and return to this component. +- Design still moving (names, packaging, two shapes): say so and keep grilling; do not draft mid-churn, since every rename re-edits every file that carries the name. + +Record provenance throughout at the four levels `intent-files/references/handoff.md` defines. + +## Finish + +When the frontier is empty: list each commitment in the human's words with its provenance, propose placement, and ask for confirmation. +A question no fact settles and the human cannot settle on paper is offered as a `variants` experiment, with candidates from the ledger, criteria, and budget; they may run it, decide now, or say "no commitment here". +Record a question sent to a run as pending; when its brief returns, the human adopts, rewrites, or rejects a candidate sentence before anything is recorded in the draft. +Then draft, write the handoff note from `intent-files/references/handoff.md`, and hand the assembled file back for the human to confirm ordering, placement, and tidying. diff --git a/.agents/skills/intent-drift-audit/SKILL.md b/.agents/skills/intent-drift-audit/SKILL.md new file mode 100644 index 00000000..9f34113d --- /dev/null +++ b/.agents/skills/intent-drift-audit/SKILL.md @@ -0,0 +1,59 @@ +--- +name: intent-drift-audit +description: Audit code, a diff, or a PR against the applicable INTENT.md, *.intent.md, or CONTRACT*.md and report conforms, violates, partial, not-yet-realised, or cannot-verify per statement with file:line evidence, ready for a PR comment. Use once an implementation claims to realise its intent, for conformance checks, automated review, or before modifying governed code; not mid-implementation. +--- + +# Intent drift audit + +Intent files are defined in `intent-files`; they and contracts hold even when tests pass, so this audit reads code against each statement with evidence instead of trusting the suite. +It reports; it never edits intent or contract and never silently fixes code during a review. + +## Scope + +Paths under audit: the diff, the named component or module, or the subtree. +State which revision you audited; if the checkout may be behind, fetch intent and code from the remote default branch or the PR head. +Applicable intent is what `intent-files` Placement makes cumulative for each audited path. +Where a statement says "see `CONTRACT.<name>.md §<section>`", the contract section is the statement; when the user names a contract, audit an implementation against it directly, treating each item `contract-authoring` requires per operation as a statement and the conformance suite's coverage of each as part of the evidence. + +## Verify + +Split each file into normative statements, one per bullet or sentence, skipping purpose prose; a sentence about an external producer or environment is context for the statements that depend on it, not a statement. +Classify by meaning, not position: a sentence that constrains the software is a statement wherever it sits, including a glossary definition that carries its rule ("Eject removes credential material from memory, best effort"); only purely explanatory text is context. +Statements in an ancestor file that govern code outside the audited paths are out of scope: skip them and count them in the header. +Give each an ID of its section's anchor, or its number or heading where it has none, plus an ordinal (`CONTRACT.agent-identity.md#agent-identity-channel-2`, `xtask/INTENT.md#requirements-1`), so a later audit can diff against this one even after a heading is reworded or a section renumbered. +A statement is a target, not a requirement, only when it describes behaviour as not yet built ("TPM keys are wanted later"); "for now" on a present constraint ("remains a CI-only check for now") and a `NOTE:` stating an anti-requirement are requirements today, so read the sentence, not the marker; realising a target anyway is worth a note. +It runs on a merged tree or a branch whose implementation is finished; mid-implementation, say so and stop, because every unfinished statement would read as a violation. +For each: cite `path:line`; decide **conforms**, **violates**, **partial** (honoured on some paths, configurations, or the primary path and bypassed on another), **not yet realised** (a deferred target), or **cannot verify** (say what is missing); for a diff, mark **introduced**, **fixed**, or **pre-existing**. +Statements that need design judgement rather than code reading are audited only when the diff plainly bears on them, and labelled as judgement, since they have no `path:line` to cite. +Note `INVARIANT:` and `UNTESTED:` comments that relate; they are the code's own claims. +"Probably fine" is cannot verify: a status of conforms needs `path:line` or a check that establishes the property, not the absence of an obvious counterexample. +When a statement's span is feature-gated or build-specific, check each variant. + +Also report **intent rot** (names, paths, sections the intent cites that no longer exist, and plan tense for behaviour that now exists) and **not covered** (behaviour no statement decides: in a diff, new public surface, dependency, fallback, or configuration; in a module audit, any path that looks like a design choice or a latent defect). + +## Report + +```md +## Intent drift audit: <scope> + +Audited against: `<file>` (<n> statements, <m> out of scope), ... + +| ID | Statement (abridged) | Status | Evidence | Note | +|---|---|---|---|---| +| xtask/INTENT.md#dependencies-1 | Only lexopt, anyhow, xshell, tinyjson | Violates (introduced) | `xtask/Cargo.toml:14` | adds `serde_json` | + +### Violations +### Partials +<Per entry: what intent requires, what the code does, smallest conforming change as a proposal.> + +### Cannot verify +### Not yet realised +### Intent rot +### Not covered by intent +``` + +Lead the table with violations and partials, then cannot verify, then not yet realised, then conforms; drop empty sections. +A clean audit is two lines plus the table. +When the human wants the mapping kept, the table becomes an `EVIDENCE.md` beside the intent, one row per claim with the directive, intent line, and code paths that realise it and `[unsure]` on inferred rows. + +When the audit runs because you are about to change governed code, a violation you would introduce is a stop: surface the statement ID, evidence, and options to the human. diff --git a/.agents/skills/intent-files/SKILL.md b/.agents/skills/intent-files/SKILL.md new file mode 100644 index 00000000..5c615103 --- /dev/null +++ b/.agents/skills/intent-files/SKILL.md @@ -0,0 +1,66 @@ +--- +name: intent-files +description: Discipline for INTENT.md and *.intent.md files - the durable design decisions and rationale behind a component; what belongs there versus CONTRACT*.md or AGENTS.md, where they go, how to word statements so they stay true, and how cited sections keep stable anchors. Use when drafting or revising intent files, when anchoring a section for citation, or when another intent skill needs the vocabulary. +--- + +# Intent files + +An intent file holds the design commitments a human has made durable: each one rules out a plausible, locally correct change that was deliberately rejected. +It is authoritative where present and optional everywhere; `AGENTS.md` constrains how contributors work, `CONTRACT.md` or `CONTRACT.<name>.md` exists only for a surface that two independently built parties must agree on. +A component's own interface (its CLI, config, storage) with the reason it is shaped so is intent, not contract, until a named second party depends on it. +If the repository has `docs/intent-files.md`, it is the house convention. + +An agent never writes intent from code or a brief, and never creates, edits, or deletes an intent file on its own; it may interview the human (`grill-intent`), record the commitments the human states or adopts, tidy them to these conventions, and hand the assembled file back for the human to confirm, since ordering, placement, and tidying are the agent's and the human has seen only the sentences. +Write drafts where the user says, otherwise outside the source tree, since an intent file in the tree is authoritative the moment it exists; say where. +When code, tests, contract, and intent disagree, report the divergence; the human decides which is wrong. + +## Admission + +A statement belongs when all three hold: + +> Would the software be wrong if tests passed but this were violated? +> Does it rule out a plausible change a human deliberately rejected, rather than restating the contract without naming the rejected alternative? +> Could a reviewer cite it to reject a change? + +If it constrains how a contributor performs the work rather than the resulting software, it is `AGENTS.md`. +"Adding a variant requires an explicit decision" is process; the intent is "a variant without an explicit mapping is rejected, never given a fallback". +A test can enforce a property without recording why it exists; the intent beside it says what the test protects, so that an agent facing a red check escalates instead of deleting the test. +Default to no file: create one only when at least one commitment passes. + +## Placement + +`INTENT.md` at the narrowest directory covering all governed code; `<name>.intent.md` beside one file when a single module carries a policy its component does not. +Applicable files are the `INTENT*.md` in the directory and its ancestors plus the `<name>.intent*.md` beside the module itself, cumulative, and a specific file never overrides a broader one, so a narrowing is a contradiction a review catches rather than a silent exception; nothing follows a link, since a link breaks on rename and confers no applicability. +A design shared by several components is a part file, `INTENT.<feature>.md` or `<module>.intent.<feature>.md`, present in each governed directory; a symlink to one source keeps the parts identical, and where a symlink is not possible the copies are one text and a difference between them is a defect. +A part is read from several directories, so it names repository paths as code (`docs/agent-identity/CONTRACT.agent-identity.md`) rather than relative links. +A file per source file is the exception, never the default, and never mechanical. + +## Anchors + +A section that tests, code comments, or other documents cite or will cite carries an anchor on its own line just before its heading, and citations name the anchor rather than the heading text or number, because rewording a heading or inserting a section changes those and breaks every citation. +The anchor is `<a id="<scope>-<topic>"></a>`, where `<scope>` names the file's component, feature, or contract (`<a id="agent-identity-channel"></a>`) and no other section in the repository uses the id, copies of a part file aside, so a search for it finds only the section and its citers. +An anchor keeps its id once published, even when its heading is reworded or its section renumbered; removing one breaks every citation of it, so the change that removes it updates them. +A file whose rules a test may cite has at least one heading, with an anchor on each cited section, rather than one flat list, because a flat list leaves a citation nothing narrower than the whole file. + +## Shape + +No mandatory sections, no title (the directory names the component), and no length budget; one commitment may be the whole file, and intent is read on demand by directory, like a doc comment, not loaded on every task. +A flat list for a handful that nothing cites; topical headings only when they aid retrieval or carry an anchor; `Overview` or `Purpose` only for a non-obvious role; `Non-goals` only for plausible, deliberately excluded behaviour. +A reason accompanies a commitment when it explains why a tempting alternative stays excluded; related commitments may share one, and reasons live in the same file as the commitments they support, so that two documents cannot drift apart. +One meaning per verb, because that is what lets a reader tell a requirement from advice: the declarative present or an imperative states a requirement, `should` a recommendation, `may` a permission, `can` a capability; never addressed to agents, since that constrains the contributor, not the software; sentences follow `prose-writer`. +Everything written normatively is to be realised by the time the branch merges; behaviour that is not yet to be built says so in the sentence ("TPM keys are wanted later"), so an implementer knows not to build it and a reviewer knows not to expect it. +Distinguish a present constraint that may change ("remains a CI-only check for now"), a deferral ("wanted later"), and a non-goal ("the intention is not to"); the first two are read differently by an audit, and the third tells a reader not to expect change. +State precision in the wording (`exactly these`, `at least`, `may vary provided`, `is permitted to`) rather than labelling the file exact or broad. +Give a conditional commitment a trigger or state and one response ("when a token is deleted, …", "while a rotation is in progress, …", "if the response is lost, then …"); one condition and one outcome per sentence is what a reviewer can check and an audit can split. +Scope every `never`, `only`, `always` to the boundary it protects and name the configurations a conditional commitment applies to, because a sibling, feature flag, or build across that boundary falsifies the unscoped form. +A scope sentence earns its place only when placement leaves a likely misapplication, stated once, next to the commitment it qualifies. +When the component introduces its own terms, a glossary entry per term opens with one or two sentences saying what it *is*, continues with its commitments, and ends with an example where the boundary is unclear and an *Avoid:* line where a competing word exists; a heading per term when entries carry commitments or will be cited, a flat `**Term**:` list otherwise; commitments that outgrow an entry move to their own section with a pointer from the entry. +A term belongs when this component gives it a meaning a reader could not have guessed, common word or not; a general programming concept used in its usual sense does not. +Specifics the human wants pinned (an exact set of variants, a dependency allowlist, a permitted tool, a pipeline order, a CLI's selector semantics) are intent even though they look like implementation; put them under the glossary entry of the concept they refine, or in a `<module>.intent.md` beside the one module they govern, and word them as the set or the permission. +The file still does not describe the implementation: incidental detail is the first thing a reviewer questions. +When a file grows, cut first what fails Admission. +A commitment that will be cited in reviews may carry a short name ("the single-classification rule") so it is referred to, not restated. + +`references/pitfalls.md` lists how drafts have failed review and what the maintainer said; read it before handing a draft back or when reviewing. +`references/examples.md` points at the file the maintainer wrote by hand and says what makes it work; read it when unsure what a finished file looks like. +`references/handoff.md` is the handoff note and provenance table to write when handing drafts back. diff --git a/.agents/skills/intent-files/references/examples.md b/.agents/skills/intent-files/references/examples.md new file mode 100644 index 00000000..ab293627 --- /dev/null +++ b/.agents/skills/intent-files/references/examples.md @@ -0,0 +1,19 @@ +# Examples the maintainer wrote + +Only files the maintainer has confirmed as examples appear here. +Read the file at the path given; what follows is only what makes it work. + +## `xtask/INTENT.md` (Devolutions/devolutions-gateway, branch `cbenoit-build-xtask-checks`, unmerged at the time of writing) + +Until that branch merges the file is not on the default branch, and the description below stands on its own; the path is updated when it lands. +Written by hand for a new crate and committed as written, about 100 sentences. +No title; `## Overview` holds the purpose in one line, then non-goals ("The intention is not to rewrite ci.yml in Rust", "Do not preserve tlk.ps1 as an abstraction") and deferrals ("For now, ignore Windows installers", "remains a CI-only check for now") side by side, each one sentence. +`## Dependencies` is a four-crate allowlist plus a goal ("compile under one second"): an inventory that is itself the decision. +`## Pipeline` pins the stage order as an arrow diagram and numbered steps, and calls out the one ordering that matters (`canonicalize -> deduplicate -> apply declared coverage -> merge -> order`): implementation shape, deliberately chosen. +`## CLI` gives the literal usage text and the selector semantics in four sentences ("Repeated check names form a union", "A selector that unexpectedly matches nothing is an error"): an exact contract inline, because nothing implements this interface independently. +`## Concepts` is a glossary, one `###` per term, one to three sentences with an example; the rules live inside the definitions ("A missing requirement is an error, not an applicability skip", "Prerequisites must not be used to install missing requirements implicitly") and `NOTE:` marks a future direction or an anti-requirement. +Voice mixes imperative and declarative; under the one-meaning-per-verb rule its "`cargo xtask` should remain a developer-focused tool" reads as a recommendation, and no sentence is about agents. +Beside it, `xtask/AGENTS.md` is seven lines of module-organisation policy for contributors ("Prefer vertical task modules", "Do not add or modify dependencies without explicit approval") and nothing else, and `xtask/EVIDENCE.md` maps each design claim to the directive, intent line, and code path that realise it, with `[unsure]` on inferred rows. + +The same author's `xtask/src/model.intent.md` shows the one thing to avoid: firm wants (the exact `Subject` variants; permission to use `cargo metadata` for discovery) written as "subjects I have in mind" and "may be used" because their placement was unclear. +The set belongs under `### Subject` as "exactly these", the permission as "discovery is permitted to use `cargo metadata`", and the placement doubt in the PR. diff --git a/.agents/skills/intent-files/references/handoff.md b/.agents/skills/intent-files/references/handoff.md new file mode 100644 index 00000000..5c19eb19 --- /dev/null +++ b/.agents/skills/intent-files/references/handoff.md @@ -0,0 +1,39 @@ +# Handoff note + +Written with a set of intent drafts so the human can confirm the assembled file. +It is a session artifact: save it in the harness's artifact area when one exists, otherwise outside the source tree, and never stage or commit it. +When the drafts go up in a pull request, its content becomes a PR comment, not part of the body, because the body becomes the squash commit and the changelog. + +```md +# Intent drafts: handoff + +<Code revision the interview read.> + +## Provenance + +| # | Commitment as written | Human's words (if tidied) | Level | +|---|---|---|---| + +## Left out on purpose + +<What came up and went to AGENTS.md, a contract, another component's intent file (named), or the PR instead, so it is not asked again.> + +## Placement + +<Which file each commitment landed in, and which directories symlink to a shared part.> + +## Pending experiments + +<Questions sent to a `variants` run, each with its candidates; when a brief returns, only a sentence the human adopted or rewrote is recorded, with its provenance level.> +``` + +Levels: + +| Level | Meaning | Where it goes | +|---|---|---| +| 1 | The human's own words, quoted verbatim | Intent text | +| 2 | The human's words tidied to the conventions, original quoted beside | Intent text | +| 3 | Agent wording the human adopted: an option label or candidate sentence they copied, or a delegated choice they approved | Intent text, marked so the record shows the human decided but did not word it | +| 4 | Filled in by the agent while drafting, never stated or adopted by the human | Listed here for the human; never written as intent | + +An open question at handoff means the frontier was not empty; ask it before handing off rather than listing it here. diff --git a/.agents/skills/intent-files/references/pitfalls.md b/.agents/skills/intent-files/references/pitfalls.md new file mode 100644 index 00000000..adaa9a6e --- /dev/null +++ b/.agents/skills/intent-files/references/pitfalls.md @@ -0,0 +1,72 @@ +# How intent drafts fail review + +Each entry is a mistake that reached review, the correction, and the rule it taught; the maintainer's words are quoted where they exist. +Use it as the checklist when reviewing or before handing a draft back. + +## Process dressed as intent + +- "Adding a new `RecordingFileType` requires an explicit decision" constrains the contributor, not the software. + The commitment is "a variant without an explicit `/shadow` mapping is rejected, never given a fallback streamer". +- Stages, decision IDs, agent roles, PR sequencing, and test-writing workflow appeared in drafts. + "Just the spec, and implementation decisions, nothing on the agent operational side." +- A testsuite line referred to agents; the maintainer reworded it "so it no longer refers to agents". ([picky-rs #556](https://github.com/Devolutions/picky-rs/pull/556#discussion_r4238310499)) + +## Plan or ticket, not commitment + +- "The intent file should be normative. 'We would like to...' only fits behavior we intentionally haven't implemented yet, and this behavior is already implemented." ([devolutions-gateway #1942](https://github.com/Devolutions/devolutions-gateway/pull/1942#discussion_r4157014381)) +- "This sounds like a ticket body. Write in present tense, in a normative way, describing how things are expected to be at any point in time." ([#2003](https://github.com/Devolutions/devolutions-gateway/pull/2003#discussion_r4129433952)) +- Firm wants written tentatively because their placement was unclear: "I'm not sure yet what is the correct place to document these, but here are subjects I have in mind" and "`cargo metadata` may be used". + The set and the permission were requirements; state them as such, and put the placement doubt in the PR rather than the file. +- A future direction is allowed as a short, labelled "Later, X could …" note, or as a constraint ("V1 must not preclude this migration"). + +## Not true, or not true everywhere + +- "'Each client begins at its own live edge' isn't always true with the current implementation. Can we state the actual rule here?" ([#1942](https://github.com/Devolutions/devolutions-gateway/pull/1942#discussion_r4198134333)) +- "Every library imports SPKI": ring has no SPKI parser; it became "the contents of the SPKI `subjectPublicKey`". +- "Fill-in providers implement what no library offers": aws-lc offers the KDFs; it became "what some backend's library lacks". +- "The agent never sends its certificate to authenticate": a planned mTLS path sends it to another component; it became "to the authority". +- "The bundle's `fips()` is false": a bundle holding only an aws-lc FIPS build falsifies it; it became "a non-FIPS convenience that claims nothing". +- "It's 2026.3.0, specifically; because 2026.3.1 is also 2026.3 and it will have a different format." ([#2039](https://github.com/Devolutions/devolutions-gateway/pull/2039#discussion_r4221382200)) +- "Either drop the backward-compatibility claim or narrow it to 'Clients receive no `SegmentStarted` message before the first segment'." ([#1942](https://github.com/Devolutions/devolutions-gateway/pull/1942#discussion_r4157014381)) +- "A hand-written input never expects success" broke when boundary controls needed acceptance; it became three lines naming the exceptions. +- "`Unsupported` is allowed above the must-support set" missed inputs below it; it became "outside". +- "p = q" in an inconsistent-key rule was one RSA crate's behaviour; it became "components that are not a consistent RSA private key for (n, e)". +- A capacity-limit rationale cited stack growth during decoding; measurement showed key construction grew the stack. + A reason must be as true as its rule. + +## Belongs elsewhere, or nowhere + +- A rationale intent restated the token format and operations table under "decisions it must preserve"; the maintainer made the contract self-contained and kept only the choices and reasons. +- "This is verbatim the same as what is written in the README.md. INTENT.md is not intended to be the same as README.md." ([IronVNC #1542](https://github.com/Devolutions/IronVNC/pull/1542#discussion_r4013943689)) +- "I would remove this, as it's part of the file name / convention." ([#1946](https://github.com/Devolutions/devolutions-gateway/pull/1946#discussion_r3898220402)) +- "A backend is a thin adapter" and "it passes the full conformance suite" in every backend intent drew "does that belong to this crate?" and "how useful is that invariant?"; both were cut. +- Scope / includes / excludes / "Component boundaries" / a closing "Boundary" note said the same thing four times in one file; one sentence next to the rule it qualifies would do. +- A table of file types with their extensions duplicated the mapping the same file said lived in code; the accepted-versus-rejected policy was the intent, the extension column was not. + +## Enforced but unwritten + +- "The actual wire contract isn't written down. Suggest rewriting it as: inputs, normalized output guarantees, and an exact Start/Pull/response transcript." ([#1942](https://github.com/Devolutions/devolutions-gateway/pull/1942#discussion_r4156764692)) +- "`gateway-db` must never depend on crates that implement feature behavior. Adapters may depend on `gateway-db`, but never the other way around. Please add this to the invariants in `crates/gateway-db/INTENT.md`." ([#2039](https://github.com/Devolutions/devolutions-gateway/pull/2039#pullrequestreview-5457434157)) +- "Write some human-vetted invariants about the concrete invariants and intentions you encoded when authoring this PR." ([#1946](https://github.com/Devolutions/devolutions-gateway/pull/1946#pullrequestreview-5018598898)) + +## Rule without its reason + +- "`fips()` always returns `false`" drew "explain why"; it gained "because RustCrypto has no FIPS 140 validated module, so the FIPS policy rejects any provider containing its entries". +- "It's good to explain we don't want to turn that into a generic LLM client. Also, I would document that a vendored implementation is intentional to reduce dependencies." ([#2005](https://github.com/Devolutions/devolutions-gateway/pull/2005#discussion_r4156224738)) + +## Files that disagree + +- "The two intent files now disagree. Both are human-owned, so the wording needs the maintainer's approval." ([picky-rs #572](https://github.com/Devolutions/picky-rs/pull/572#discussion_r4282323307)) +- After "thin adapter" was dropped, "the thin-adapter rule applies to backends only" pointed at nothing; grep for every reference when removing a rule. +- Crate intents linked to a shared design file under `docs/`; renaming it broke four links, and a link confers no applicability under the convention. + A shared design is an `INTENT.<feature>.md` part symlinked into each governed directory. + +## Timing + +- Packaging changed three times and each time the intents, the contract, and every file naming the crate changed; drafts written mid-design needed sixteen corrections a day later. + Draft after decisions settle, and ship intent edits in a small PR ahead of the code that depends on them. +- Encodings, a thumbprint definition, and a grace-window formula were defined by the drafter and never discussed; track provenance and list filled gaps for the human. + +## Form + +- One sentence per line, typos listed exhaustively, trailing newline, no heading that repeats the folder name. ([#1942](https://github.com/Devolutions/devolutions-gateway/pull/1942#discussion_r4156764692), [IronVNC #1544](https://github.com/Devolutions/IronVNC/pull/1544#discussion_r4024059256)) diff --git a/.agents/skills/intent-review/SKILL.md b/.agents/skills/intent-review/SKILL.md new file mode 100644 index 00000000..41ffbf36 --- /dev/null +++ b/.agents/skills/intent-review/SKILL.md @@ -0,0 +1,57 @@ +--- +name: intent-review +description: Review INTENT.md and *.intent.md files or drafts for contradictions, unscoped absolutes, misplaced content, rot, and missing rationale; returns findings and proposed rewordings, never edits. Use when asked to review, check, or tighten intent files. +--- + +# Intent review + +Intent files are defined in `intent-files`; agents read them literally before every change in their directory, so a loose sentence becomes a wrong implementation. +A proposal rewords a commitment the human already holds; when a finding can only be fixed by deciding what the software should be, ask instead of proposing. + +## Gather + +State which revision you reviewed; if the checkout may be behind, fetch from the remote default branch and say which findings apply to which version. +Collect every file `intent-files` Placement makes applicable and the `CONTRACT*.md` it defers to; contradictions mostly live between files, and copies of a part file that differ are a finding on their own. +Read the governed code, manifest, features, and configurations; most false absolutes are found there. +When a sentence feels wrong and you cannot name why, `intent-files/references/pitfalls.md` is the checklist. + +## Lenses + +Apply each to every normative sentence and quote the sentence in the finding, so the author finds it after edits. +Four probes decide most of them: is this authoritative or only explanatory; would changing it require a design decision; would it still matter after a rewrite; what undesirable implementation could still satisfy it. + +- **Contradiction**: two statements that cannot both hold, including a specific file quietly narrowing a broader one. +- **Not a commitment**: rules out no plausible change, would not survive a rewrite, or restates code, manifest, README, path, contract, or a sibling without adding a constraint or reason not recoverable there; at section level, scope prose that placement already supplies, boundary notes repeated, headings for a handful of bullets nothing cites, inventories whose owner is elsewhere; "does that belong to this component?" +- **Still satisfiable badly**: a commitment an undesirable implementation meets as written; propose the wording that excludes it. +- **Process in intent clothing**: "requires a decision", "run X", anything about agents or contributors; propose the software property behind it, or `AGENTS.md`. +- **Plan or tentative tense**: "we would like", "will be extended", "later" for behaviour that already exists, since a realised commitment must read as normative; "I have in mind", "may be used" for a firm want; propose the normative form or the explicit permission. + A deferral the code has not realised is correct as written, not a finding. +- **Schedule on parties**: a party or example named with a stage ("A now, B later" for "e.g.: A, B"); timeline is not intent. +- **Unscoped absolute**: `never`, `always`, `only`, `every`; which boundary, and does a sibling, feature flag, backend, or build falsify it? +- **Overstated**: `must`, "if and only if", "exactly these", an exhaustive list, with nothing showing the strength is intended; ask before proposing the weaker form. +- **Unverified universal**: "every library does X" with no cited check. +- **Code disagrees**: the code does not do what the sentence says; report it as a divergence for the human, not as a defect in either. +- **Enforced but unwritten**: a wire format, state machine, or dependency direction the code enforces that no document states; propose where it belongs. +- **Incidental detail**: a helper name, layout, or one library's behaviour or number stated as a rule with nothing showing a human chose to pin it; a deliberately pinned detail with its reason is a commitment, so ask rather than flag, and an author's insistence that it is intentional settles it. +- **History for rationale**: "previously Gateway was stateless, which caused problems with deletion" where the intent is "deleting an agent survives Gateway restarts"; propose the property. +- **Missing reason**: a rule a contributor could plausibly "fix" because no alternative is named as excluded. +- **Not checkable**: "handles", "supports", "properly", a condition without a stated outcome, or two outcomes in one sentence; propose an observable with a bound. +- **Anchor**: a section a test, code comment, or document cites without an anchor, cited rules in a flat list with no heading, a published anchor renamed, or an anchor removed while citations still name it, since each breaks or blocks a citation (`intent-files` Anchors); search the tree for the file's name to find the citers. +- **Hygiene**: links and anchors that do not resolve, removed rules still named, a synonym the glossary avoids, duplicated sentences across siblings, house style. + +## Report + +Order by consequence: contradictions and false absolutes, then not-a-commitment and process, then anchors and hygiene. + +```md +### <N>. <lens> — `<file>` + +> <quoted sentence> + +<Why, citing code or another file when relevant.> + +**Proposed:** <reworded sentence>, or move to `<artifact>`, or delete. +``` + +Close with what is strong as written, the questions only the human can settle (two plausible readings the code does not decide, which of two files is right, what they want preserved where a proposal would change the design), and a one-line verdict: adopt, adopt after corrections, or rework. +A clean file gets a short report. diff --git a/.agents/skills/lead/SKILL.md b/.agents/skills/lead/SKILL.md new file mode 100644 index 00000000..73e79a44 --- /dev/null +++ b/.agents/skills/lead/SKILL.md @@ -0,0 +1,47 @@ +--- +name: lead +description: Deliver one pull request from a brief with sub-agents - workers, a fresh-context reviewer on a frozen commit, your own verification, and a report the reader can verify. Use when a change needs its own session and more than one pair of hands, assigned by an orchestrator or by the user. +--- + +# Lead + +You own one pull request from brief to merge-ready and are its last check; a second pull request gets a second session, so each report, review loop, and branch has one owner. + +## Start + +Read the brief, then every applicable intent file, `CONTRACT.md`, and `AGENTS.md` for the paths in scope. +Models: those the brief names, else ask the user before the first sub-agent, recommending from the options you can see - strong for reviewers and normative work, and a reviewer family different from the writer's when available, since one family shares its blind spots. +Write the plan as an artifact outside the tree, never committed, with the acceptance criteria restated as checks you will run. + +## Decisions + +A decision the brief, intent files, and contract do not settle is not yours: escalate with options, how each could go wrong, and a recommendation, and let no worker encode it in a test or code meanwhile, since whatever lands first becomes the text the rest is bent to. +Within autonomy - naming inside repository conventions, test structure, which published vector, a classification a stated rule decides, non-semantic documentation - decide, record in the plan, flag in the report. +Two approaches within autonomy with trade-offs paper cannot settle are a `variants` tournament, run as sub-agents before committing to either. +A change to an intent file, `CONTRACT.md`, or blind suite that this pull request does not own never rides it: escalate, and wait or stack on the pull request that carries it. + +## Sub-agents + +One task per sub-agent, briefed from an artifact per `references/templates.md`, pointing at paths rather than copying them so the sub-agent reads current text, and listing what it must not read as well as what it must, because isolation is only what the brief denies. +Workers write code and tests in your worktree or branch, each behaviour through `tdd` or against the blind suite's failing checks where one exists, and never push or open the pull request; testers follow `test-writer`, suite authors `blind-testsuite`. + +## Review loop + +Review a commit, not a working tree: the worker commits, hands over the range, and stops editing until the verdict, since a change during review means the reviewer judged something that no longer exists. +The reviewer gets the range, the acceptance criteria, the applicable intent, contract, and `AGENTS.md` paths, and an instruction to run `code-review`; never the worker's transcript or reasoning, because the writer's framing anchors a reviewer before it reads the evidence. +Blocking findings go back to the worker; non-blocking ones are fixed or the reason not to is recorded in the plan. +Set the round cap in the plan before the first round; at the cap, outstanding fixes go to a fresh reviewer rather than out unreviewed, because post-cap fixes are exactly where unreviewed defects land. +A round that reintroduces an earlier finding ends the loop: escalate with both findings. +After the last feature edit and before the last review round, a worker runs `dependency-pruner` when a manifest or lockfile changed, then another runs `test-pruner` in its editing mode on the diff, in that order because a replaced package changes the code the mutants measure; the final range the reviewer sees includes their commits, and the pruner reports travel with it, since `dependency-reviewer` reads the dependency report as the evidence that an addition is load-bearing. +Post both pruners' evidence once the pull request exists. + +## Your verification + +Verdicts are claims; verify before opening the pull request, whatever the loop returned: build, run the full suite, read the whole diff, and read line by line any code handling untrusted input, keys, authentication, or a `CONTRACT.md` surface. +Check each acceptance criterion against evidence you produced, byte-compare approved normative text copied into the tree against its approved artifact, and run `intent-drift-audit` on governed paths, since the realisation is now claimed complete. + +## Pull request and report + +Open the pull request per the repository's conventions, and put the acceptance evidence, deviations from the brief, and open questions in a comment, because a body that becomes the squash commit carries rationale, not process; run `review-feedback-loop` once findings arrive. +Report per `references/templates.md`: head SHA and CI run let the reader verify instead of trust, and every non-success job is named, because a summary count hides the one that matters. +Merging is the human's: report, mark the status waiting, and stop rather than polling. diff --git a/.agents/skills/lead/references/templates.md b/.agents/skills/lead/references/templates.md new file mode 100644 index 00000000..c1175ee3 --- /dev/null +++ b/.agents/skills/lead/references/templates.md @@ -0,0 +1,35 @@ +# Lead templates + +Session artifacts: save in the harness's artifact area, never commit. + +## Sub-agent brief + +```md +# <task id>: <title> + +Role: worker | tester | suite author | scout | reviewer. +Goal: <one sentence>. +Acceptance: <checks the lead will run, one per line>. +Scope: <paths the sub-agent may change; none for a scout or reviewer>. +Read first: <applicable INTENT*.md, *.intent*.md, CONTRACT.md, AGENTS.md paths>. +Do not read: <paths excluded for this role, or none>. +Do not: push, open a pull request, or edit outside Scope. +When done: <worker, tester, suite author: commit, report the range and what you could not do, stop editing until the verdict | scout, reviewer: report findings, no edits>. +``` + +## Lead report + +```md +# <pull request>: report <n> + +Head: <SHA>; CI: <run URL>. +Non-success jobs: <each by name, or "none">. +Tests: <counts per suite or platform>. +Changed since last report: <file:line summary>. +Review: <verdict quoted verbatim, round count>. +Decided within autonomy: <each, one line>. +Deviations from the brief: <each with reason>. +Decision needed: <question>; options (a) (b) (c) with how each could go wrong; recommendation. +UNTESTED: <properties, or "none">. +Status: waiting | blocked on <what> | ready. +``` diff --git a/.agents/skills/orchestrate-effort/SKILL.md b/.agents/skills/orchestrate-effort/SKILL.md new file mode 100644 index 00000000..74f055d6 --- /dev/null +++ b/.agents/skills/orchestrate-effort/SKILL.md @@ -0,0 +1,54 @@ +--- +name: orchestrate-effort +description: Coordinate a multi-pull-request effort - one lead session per pull request, a record of current rules and decisions, human gates on intent and contract, and verification of every report. Use when a feature spans several pull requests, needs a contract between independently built parties, or a suite written blind to the implementation; a standalone pull request takes `lead`. +--- + +# Orchestrate an effort + +You plan, brief leads, relay decisions, verify reports, and keep the state; you never write production code, because the one who holds the plan cannot also be the independent check on its realisation, so a change you want goes to a lead, and merging is the human's. + +## Order of work + +1. `grill-intent` with the human, structure first (units, packaging, names), because drafting intent while these move re-edits every file that carries a name. +2. `contract-authoring` and `contract-review` when a second party must agree; the contract is its own pull request, approved before anything is built against it. +3. `blind-testsuite` as its own pull request when there is a contract, written from it before any implementation. +4. Implementation pull requests, one lead each, stacked on the suite where they must run it. +5. `intent-drift-audit` over the whole realisation after the last merge, with a summary of the decision log for the human. + +Keep the contract and suite leads alive while implementations run against their open pull requests, so an escalation from an implementation lead becomes your decision and a message to the lead that owns the text. +A change to a contract, an intent file, or the suite goes to the pull request that owns it - the open one through its lead, or a new one once it has merged - never into the implementation pull request, because text amended to fit one implementation stops governing the others. + +## Models + +Unless the user has already said, ask before creating leads or sub-agents which models to use, recommending from the options you can see: strong for leads and anything normative, cheaper for scouting, a reviewing family different from the writing one when available; record it in the state. + +## Human gates + +Two are scheduled: the intent is adopted, and the contract, when there is one, is approved before the suite. +Everything else is decided within the current rules, logged, and reported for the human to veto; a decision log read once costs less than a question per entry. +Stop and ask only for a change to intent or contract text, a public API or dependency outside the plan, a security trade-off, or a scope expansion. +A decision the human has not committed and no fact settles, or an escalation with alternatives, is a `variants` experiment: report its start and proceed within the budget the rules set, and the human decides from its brief rather than from a question. +A question carries background, options with how each could go wrong, and your recommendation; a placement or formatting choice is a default plus "say if not"; small items are batched, each with its recommendation. +Approved wording is final unless new evidence shows it false or contradictory, since each re-presentation costs the human a context switch and churns text leads already act on. + +## Leads + +One lead session per pull request, few enough open that you verify each report yourself. +Draft the brief as an artifact and send it as the kickoff, since the kickoff is the lead's whole context: goal, scope, acceptance criteria, applicable intent, contract, and `AGENTS.md` paths, isolation rules, models, what to escalate, and the report template from `lead`. +Leads act on the current rules only, never on the decision log. +A message to a lead quotes the human's words verbatim marked user-directed, numbers its items, lists the required steps, and ends by asking for head and CI; your inference is never presented as the human's authorization. + +## Reports are claims + +Verify each report with the tool, not its text: head SHA, CI rollup with every non-success job, unresolved review threads, diff against the base, byte-compare of approved normative text against its artifact. +A report without a verifiable head goes back. + +## State + +Keep the state as the artifacts in `references/state.md`, outside the tree and never committed, because committed text carries rationale, not process. +Update the current rules in the same edit as the decision that changes them; rules that lag the log are rules leads act on wrongly. +You assign every decision and escalation identifier; leads that number their own collide. +After compaction or a pause, re-read the current rules and verify live state (fetch, pull request, CI) before acting; if the state artifacts cannot be found, say so and stop, never reconstructing decisions from memory. +A lead that lost context gets its brief again and a short message listing what it may act on, ending with the same rule. + +The nouns here - session, sub-agent, message, artifact area, kickoff - and how each harness provides them are in `references/harness.md`. diff --git a/.agents/skills/orchestrate-effort/references/harness.md b/.agents/skills/orchestrate-effort/references/harness.md new file mode 100644 index 00000000..d15ff4b9 --- /dev/null +++ b/.agents/skills/orchestrate-effort/references/harness.md @@ -0,0 +1,11 @@ +# Harness mapping + +The skills use five nouns; each harness provides them differently. + +| Noun | Meaning | GitHub Copilot app | Claude Code | Other harnesses | +|---|---|---|---|---| +| Session | One agent with its own context and checkout; leads one pull request | A project session with its worktree, created with `create_session` | A `claude` process in its own `git worktree` | A thread (Delta) or equivalent; one per pull request | +| Sub-agent | A task run in a fresh context that returns a report | The task tool (`explore`, `general-purpose`, `rubber-duck`), addressed with `write_agent` | The `Task` tool | Whatever runs a prompt in a fresh context | +| Message | Text delivered as a turn in another session | `send_session_message` | None built in; use a file in the artifact area or the human | - | +| Artifact area | Storage outside the tree that survives the session and is never committed | The session's `files/` under `.copilot/session-state/<id>/` | A directory outside the repository, named in the brief | - | +| Kickoff | The first prompt a new session receives; its whole starting context | The `kickoff.prompt` of `create_session`, in plan or autopilot mode | The first prompt, or a `CLAUDE.md` in the worktree | - | diff --git a/.agents/skills/orchestrate-effort/references/state.md b/.agents/skills/orchestrate-effort/references/state.md new file mode 100644 index 00000000..bcc6db7d --- /dev/null +++ b/.agents/skills/orchestrate-effort/references/state.md @@ -0,0 +1,38 @@ +# State artifacts + +Several small files rather than one, so each is read whole and edited without anchors. + +| Artifact | Holds | Who reads it | +|---|---|---| +| `rules.md` | Current rules by topic: process, intent, contract, suite, per-stage plan and acceptance, experiment budget (variants per run, runs per effort, or none); authoritative; a rule resting on an unresolved experiment is marked provisional with its brief, guides implementation and not merge; updated in the same edit as the decision that changes a rule | Leads and you | +| `decisions.md` | Numbered log `D1..`, each with date, who decided, the decision, and its rationale; a provisional entry says so until the human adopts or reverses it; a superseded entry keeps its text and gains `superseded by Dn` | You | +| `escalations.md` | Numbered `E1..`, each with the lead, the question, the options, and the decision it led to | You | +| `experiments/<n>.md` | Each `variants` decision brief, as written | You, the human | +| `briefs/<lead>.md` | The kickoff sent to each lead, as sent | You, the lead | +| `reports/<lead>-<n>.md` | Each lead report, as received, with your verification result appended | You | +| `approved/<repo path>` | Every approved normative text (intent, contract) at approval time, byte-compared against the tree at every copy | You, leads before copying | +| `models.md` | The models chosen for leads and sub-agents, with the date | You, in briefs | + +## Message to a lead + +```md +User decisions (user-directed), quoted: +1. "<verbatim>" +2. "<verbatim>" + +Steps: +1. <what to do> +2. <what to do> + +Report head and CI when done. +``` + +## Context-sync message + +Sent to a lead that lost context, after its brief has been resent. + +```md +You may act on: <the current rules that apply to this pull request, by topic>. +Do not act on anything else you remember. +If you cannot find your plan artifact, say so and stop. +``` diff --git a/.agents/skills/prose-writer/SKILL.md b/.agents/skills/prose-writer/SKILL.md index 14b78e3e..b8a47514 100644 --- a/.agents/skills/prose-writer/SKILL.md +++ b/.agents/skills/prose-writer/SKILL.md @@ -7,9 +7,11 @@ description: Write concise, human-first prose with one sentence per source line. Lead with what the reader needs to know or do. Use direct sentences, concrete verbs, familiar words, and the shortest phrasing that preserves meaning. +This is plain language at the sentence level: a reader understands on first reading. Remove throat-clearing, repetition, inflated wording, and details already clear from context. Keep each paragraph focused on one idea and order ideas by reader need rather than implementation history. Preserve necessary nuance, technical terms, warnings, and the document's established voice. +A defined term stays where a common word would be less exact, and the same word names the same thing throughout. In source formats where newlines render as spaces, start each sentence on its own line and do not hard-wrap a sentence. Code comments and doc comments follow the same convention, except that a sentence wraps only where it would exceed the line-length limit. diff --git a/.agents/skills/review-feedback-loop/SKILL.md b/.agents/skills/review-feedback-loop/SKILL.md new file mode 100644 index 00000000..81054a47 --- /dev/null +++ b/.agents/skills/review-feedback-loop/SKILL.md @@ -0,0 +1,34 @@ +--- +name: review-feedback-loop +description: Work through findings on an open pull request from Copilot, a review pipeline, or human reviewers - fix what is substantive, answer a wrong finding with the governing intent or contract, one review round per change, until a round returns nothing you accept. Use when asked to address review comments or bring a pull request with outstanding findings to approval. +--- + +# Review feedback loop + +A finding is a claim about the code: verify it before acting on it, and loop until a round returns no finding you accept, since a reviewer that saw the fixes is the only evidence they held. + +## Triage + +- Correctness, security, intent, contract, or style the repository's rules decide: fix. +- Wrong, or in conflict with an intent file or `CONTRACT.md`: reply with the file and clause, change nothing. +- A decision no intent, contract, or brief states: escalate, because a fix is the least reviewed place for a decision to land. +- A change to an intent file, `CONTRACT.md`, or blind suite this pull request does not own: escalate; it belongs to the pull request that owns the text. + +A logic fix takes the path the original code took - worker, reviewer, lead verification - when a `lead` loop exists; a trivial fix (typo, wording, comment) is applied directly. + +## Reply and the next round + +Reply to each thread in one sentence with the fixing commit or the answering clause, with the repository's comment footer when one is defined. +Resolve the automated threads you fixed; leave a human's thread for the human unless the repository's convention says otherwise. +One review round per substantive change and none after a comment-only one, since a round costs minutes and returns findings proportional to the diff it sees. +Watch for the round's result - the reviewer's new review, or its check run where the pipeline posts one - and triage it when it lands, because the loop is open until someone reads the verdict. +Where a pipeline reviews every pushed head on its own, the push is the request: batch a round's fixes into one push, because each head spends one of a bounded number of reviews, and answer findings that need no code change with the pipeline's hand-off command rather than a push. + +## Stop + +- A round returns no finding, or only findings you answered without a change: record the verdict and stop; the human judges the answers, since the same diff re-reviewed returns the same claims. +- A fix that reintroduces an earlier finding: escalate with both; alternating fixes converge on nothing. +- Only a human's approval remains: report the head and stop, since a human's turnaround is unbounded and watching for it burns a session for nothing. + +Fetch before pushing; when the branch has moved, rebase onto its new head so the human's commits stay in it, and force only with lease pinned to that head, because a lease guards against pushes after your fetch, not before it. +Record the reviewer's final verdict verbatim with the head SHA it applies to. diff --git a/.agents/skills/skill-discipline/SKILL.md b/.agents/skills/skill-discipline/SKILL.md new file mode 100644 index 00000000..f5ead2cf --- /dev/null +++ b/.agents/skills/skill-discipline/SKILL.md @@ -0,0 +1,45 @@ +--- +name: skill-discipline +description: Content standard for writing and reviewing agent skills (SKILL.md and references) - every sentence earns its place, one job per skill, rationale with each rule, no ceremony. Use when drafting, revising, or reviewing a skill's text. +--- + +# Skill discipline + +A skill is loaded whole into an agent's context whenever its description matches, so each sentence costs every task that triggers it. +This skill is the test each sentence must pass; structure checks and evals belong to other tools. + +## The test + +A sentence belongs when the agent could not get it elsewhere cheaply: not from the tree, a manifest, `--help`, another skill it already loads, or its own competence. +"Integration tests exercise the CLI binary, not the library" passes; a command any agent knows does not. +Put positively: there is no way the agent learns this unless the skill says it. + +## One job + +A skill does one thing; the description names that thing and the moments it applies, in two sentences. +Distinct workflows that trigger separately are separate skills, and a skill may load the skills it builds on; a concept defined there is pointed to, not re-explained, while a lens or procedure is this skill's own even when its subject is defined elsewhere. +One sentence of premise from a dependency is allowed when it names the dependency and says what this skill does with it ("verdicts apply the test in `agents-md`"); a second sentence re-explaining it is not, and a lens may restate an authoring rule when the two skills are used independently, even if one names the other. +A `references/` file holds only material needed conditionally, named from the body with the condition that calls for it. + +## Reasons where they pay + +A rule the agent cannot see the point of is the first it drops under pressure, so a rule that is surprising, costly to get wrong, or tempting to "fix" carries its because in the same sentence or the next; a plain convention or a procedure step may stand without one, and a stated consequence serves a convention as a reason. +Prefer the reason to the imperative: "`should` is a recommendation, because one meaning per verb is what lets an audit tell a requirement from advice" over "always use `must` for requirements". +Write the mechanism, not the incident: "a second copy may drift", not "a second copy drifted and lost a code"; no stages, sessions, or decision references, since the reader is a future agent, not a participant. + +## No ceremony + +No "Overview" that restates the description, no closing summary, no mandatory headings; an example earns its place when it shows a rewrite (bad form to good) the rule alone leaves ambiguous, and one that only instances the rule is ceremony, while an inline form token or an output template is the rule's own wording. + +## Words + +Plain language at the sentence level; one sentence per line; the same word for the same thing throughout. +Terms the skill coins are defined where first used, with the words it avoids when a competing term exists. +Address the agent as the reader doing the task; never describe what agents are like. + +## Reviewing a skill + +Quote each sentence that fails the test and name which part: discoverable, re-explains a concept another skill defines, surprising rule without its reason, ceremony, two jobs, incident for mechanism, describes what agents are like instead of stating the rule (a mechanism about the artifact's readers is not this). +Ask "why" of any confident rule before accepting it; procedure steps and conventions are exempt. +Propose the shorter sentence, the deletion, or the sentence with its reason; do not edit, and do not compress past meaning. +Report what is strong as written, so it is not touched. diff --git a/.agents/skills/tdd/SKILL.md b/.agents/skills/tdd/SKILL.md new file mode 100644 index 00000000..0a2547af --- /dev/null +++ b/.agents/skills/tdd/SKILL.md @@ -0,0 +1,35 @@ +--- +name: tdd +description: Red-green loop for one behaviour at an agreed seam - failing test first, the least code that passes, refactoring outside the loop. Use for test-first bug fixes and small features, or whenever the user says TDD or red-green. +--- + +# Test-driven development + +`test-writer` decides what a good test is and `test-oracle-reviewer` where its expected value may come from; this skill is the loop around them. + +## Seam + +Name the seam before the first test: the public boundary where the behaviour is observed, taken from the intent or contract file, the bug report, or the existing public interface. +Ask the user only when choosing between seams would create or change a design decision, saying what each catches and misses. +No test goes below the seam, because a test against internals passes on wrong behaviour and fails on a harmless refactor. + +## Red + +Write one test for one behaviour, run it, and read the failure: the assertion on the behaviour must be what fails. +A compile error, missing fixture, or setup panic is not red (unless compilation failure is the behaviour under test), since it goes away once that other problem is fixed and proves nothing. +For a bug, the expected value is one `test-oracle-reviewer` would class as independent, never the output of the fixed code, which only shows the fix did what it did. +A behaviour that needs a decision no intent, contract, or report states stops the loop: ask, or run `grill-intent`, before writing the assertion, because the test becomes the specification the code is bent to. + +## Green + +Write the least code that passes this test and nothing for the test you expect next; a plainly stubbed answer is the signal for the next test, not a reason to write more code now. +Run the one test while iterating and the unit's suite before each commit, so the loop leaves only green commits. + +## One slice at a time + +One test, one change, repeat; each test responds to what the last cycle showed. +Do not write all the tests first: tests written in bulk check the shape you imagined, go insensitive to what the code does, and fix the test structure before you understand the implementation. + +## Refactor outside the loop + +Refactoring is its own commit after green, suite green before and after; a design finding from implementation or `code-review` is acted on there, so a refactor never hides behind a behaviour change. diff --git a/.agents/skills/test-oracle-reviewer/SKILL.md b/.agents/skills/test-oracle-reviewer/SKILL.md index d159c865..793f6114 100644 --- a/.agents/skills/test-oracle-reviewer/SKILL.md +++ b/.agents/skills/test-oracle-reviewer/SKILL.md @@ -1,6 +1,6 @@ --- name: test-oracle-reviewer -description: Judge whether a test's expected value is justified independently of the code under test. Use when reviewing tests added or changed in a pull request, auditing a module's tests, or whenever expected bytes, snapshots, or mock values may have been copied from the implementation's own output. +description: Judge whether a test's expected value is justified independently of the code under test, and define the `// oracle:` marker that records its source. Use when reviewing tests added or changed in a pull request, auditing a module's tests, marking a test's source, or whenever expected bytes, snapshots, or mock values may have been copied from the implementation's own output. --- # Test oracle reviewer @@ -8,7 +8,7 @@ description: Judge whether a test's expected value is justified independently of A test whose expected value came from the code under test detects change, not bugs. For each test, ask whether the expected value can be justified without running the code. -Independent sources: a spec example, an external capture, a hand-derived value, an input the test chose itself and asserts was honored, a real property such as round-trip or `encode` writing exactly `size()` bytes, or a reference model. +Independent sources: a spec example, an external capture, a hand-derived value, an input the test chose itself and asserts was honored, a named law such as round-trip, a property a document states, such as `encode` writing exactly `size()` bytes, or a reference model. Self-derived sources: a snapshot of current output, a value computed through the code or a shared helper, a production constant or message copied into the test, a mock that returns what is then asserted, `is_ok()` or `!is_empty()` on an input that no requirement says must succeed, or a restatement of what a derive or the type system guarantees. Verify instead of guessing. @@ -17,10 +17,48 @@ A value pasted from the code's output is independent once it checks out against A search-engine or AI summary is not a source; cite the primary document. Classify each test as independent, self-derived, or unknown, with evidence, and report per assertion when a test mixes classes. -When a value checks out, add a one-line citation comment to the test, such as `// <spec> §<section> example` or `// hand-derived; see PR #N`, so the next review is cheap. +When a value checks out, add its marker, as below, so the next review is cheap. When a value is wrong, report a bug; do not adjust the value or the code. Unknown is an outcome, not a guess in either direction. -In a pull request review, comment on each self-derived test with the independent source to use and leave the fix to the author. +## Markers + +Mark where an expected value comes from with `// oracle: <kind> <reference>` on the test, on a case row, or on one assertion when a test mixes sources, so one search finds every test that rests on a source. +Strong kinds name an independent source, and a test, case, or assertion marked with one is deleted only when its source changes or is shown not to decide what it asserts: + +- `spec <document> §<section>`: normative text other than a contract or intent file, such as an RFC, a Microsoft open specification, or `docs/JMUX-spec.md`. +- `contract <path> §<section>`: a `CONTRACT*.md`. +- `intent <path> §<section>`: a statement in an intent file. +- `regression <issue or crash file>`: a bug report, as a Jira key such as `DGW-123` or a GitHub issue such as `#123` or `owner/repo#123`, or a fuzz crash file under `fuzz/`. +- `reference <implementation, capture, or model>`: what another implementation or a reference model does, recorded or compared live. +- `property <law>`: a law of the code under test named by one of `round-trip`, `idempotent`, `involution`, `commutative`, `associative`, or `monotonic`. + +Write every path from the repository root, with forward slashes and exact case, such as `crates/gateway-db/INTENT.md`, because markers sit at every depth and a root-relative path reads and resolves the same from each. +A file in another repository is cited as `<owner>/<repo>@<sha>:<path>`, with `<path>` from that repository's root, such as `Devolutions/devolutions-gateway@43958682a1b2:docs/agent-identity/CONTRACT.agent-identity.md`, and `<sha>` a commit of at least 12 hex digits, never a branch or tag, because a pinned commit never changes and so resolves the same every time. +A `spec` document outside `docs/`, such as `RFC 6455` or `MS-RDPBCGR`, is named rather than given as a path. +`§<section>` names the `id` of an anchor the file places before a heading as `<a id="…"></a>`, or else a heading, in any case, or the number a heading starts with, so `§3.2` matches `3.2 Framing`. +Cite the anchor when the file has one, because it survives a renamed heading or a renumbered section. +When a pinned section changes upstream, re-check the test against the new text, then bump the pin or change the test, never the expected value alone, because a value changed without its pin cites text that decides the old one. + +A `property` law is accepted only by its well-known name, because the name is its definition, while any other property needs a written source: it takes the kind of the document that states it, and is `scaffold` until one does. +An assertion whose expected value comes from running the code under test is self-derived even over generated inputs, so it is `mutant` or `scaffold`, never `property`. +A codec's `round-trip` test never replaces its spec or capture vectors, because a round-trip passes symmetric bugs. +A hand-derived value, a checked snapshot, or an input the test chose and asserts was honored takes the kind of the source that requires it. + +Weak kinds name a self-derived source, which detects change rather than bugs, so any intentional change, such as a refactor or a new feature, may delete a test, case, or assertion marked with one without justification unless `test-pruner` protects it: + +- `mutant <faults in words>`: a test or assertion kept to catch mutants that no strong source decides. +- `scaffold`: a test or assertion written for feedback while implementing that no strong source decides. + +No `scaffold` marker merges: before the pull request does, give what it marks a strong source, keep it as `mutant` when `test-pruner` finds it alone catches a fault, or delete it. +When it checks a property or intention no document states, ask whether and where to specify it, and mark it with that source once specified, since a confirmation alone is not a source; a protected test takes this path rather than deletion. + +Mark a strong source whenever one decides the value, so a test found through a mutant but decided by a spec is `spec`, because whether a test may go depends on whether a source still requires it, not on how it was found. +An unmarked test starts as unknown: verify it as above, and turn a citation in another form into the marker it supports. +A marker is a claim: verify it like any other origin. + +## Review + +In a pull request review, comment on each self-derived test that an independent source decides, naming that source, and on each changed test without a marker; leave the fix to the author. When that source needs a small non-test change, such as an injectable deadline the test can choose, request the change. -For mutation evidence on the same pull request, use `test-pruner` in pull request mode. +For mutation evidence on the same pull request, use `test-pruner` in review mode. diff --git a/.agents/skills/test-pruner/SKILL.md b/.agents/skills/test-pruner/SKILL.md index 6601597d..ac9c7dc5 100644 --- a/.agents/skills/test-pruner/SKILL.md +++ b/.agents/skills/test-pruner/SKILL.md @@ -1,65 +1,75 @@ --- name: test-pruner -description: Prune, strengthen, and consolidate tests using oracle classification and cargo-mutants evidence, for one Rust crate or module or for a pull request's diff. Use when the user asks to remove redundant tests, dedupe or consolidate tests with rstest or proptest, audit test value, clean up AI-written tests, or get a mutation-backed review of a pull request's tests. For a quick oracle-only check of a pull request's tests, use `test-oracle-reviewer` instead. +description: Prune, strengthen, and consolidate tests using oracle classification and mutation evidence, cargo-mutants for Rust and Stryker.NET for C#, for one crate, project, or module or for one change's diff. Use when asked to remove redundant tests, consolidate tests into cases or properties, audit test value, clean up AI-written tests, finalize a change's tests before its pull request opens, or get a mutation-backed review of a pull request's tests. For a quick oracle-only check, use `test-oracle-reviewer` instead. --- # Test pruner -Scope each run to one crate or module, or to one pull request's diff, so mutation runs stay affordable and results stay reviewable. +Scope each run to one crate, project, or module, or to one change's diff, so mutation runs stay affordable and results stay reviewable. Decide from two independent measurements: whether the expected value is justified independently of the code, and which faults only this test catches. Mutation testing alone cannot tell a test that matches itself from a correct one, and oracle review alone cannot tell which tests are the sole guard for a fault. +Tool mechanics: `references/cargo-mutants.md` for Rust, `references/stryker.md` for C#; load only the language in scope. + +Editing mode is for tests you own, such as a change's diff before its pull request opens: commit the changes yourself and hand the evidence tables (oracle table, kill matrix, challenge results, survivors, and found bugs) to whoever posts them as a pull request comment. +Review mode is for another's pull request: post a review and change nothing in its branch, applying mutants and challenge patches only in the clone below. +Run mutants in a clone of their own and address its files by absolute path, since a relative path resolves against whichever checkout the shell starts in, and an applied mutant left in a shared checkout corrupts every other run there. ## Measure Invoke `test-oracle-reviewer` on every test in scope and record independent, self-derived, or unknown with evidence. -Run cargo-mutants on the scoped source as described in `references/mutants.md`, adding domain mutants from its catalogs because generic mutants miss most domain faults. -Build the kill matrix of mutant by test from the cargo-mutants logs; it needs no per-test reruns. +Run the mutation tool as the reference describes, adding domain mutants from `references/domain-mutants.md` because generic mutants miss most domain faults. +Build the kill matrix of mutant by test from the tool's outputs; only the cases the reference names need per-test reruns. +A unique kill is a mutant caught by exactly one test in scope. +Make no deletion decision about a test until some mutant of the code it exercises is caught, by it or another test, since zero unique kills over nothing caught measures nothing and the invariant then passes trivially; challenge such a test instead. For tests with several assertions, record which assertion fails for each kill. Run the scoped suite repeatedly on the clean tree, for example 15 times, to find flaky tests before trusting any kill. ## Decide -Keep independent tests; merge them into rstest cases only when they check the same property and differ only in data. +Keep independent tests; merge them into cases of one parametrized test only when they check the same property and differ only in data. Never delete an independent test on mutant evidence: the mutant sample is tiny next to the real fault space, and a spec vector costs nothing to keep. -Strengthen a self-derived test with unique kills by replacing its expected value with an independent one; if none exists, keep it and mark it `// oracle: self-derived; sole guard for <mutants>`. -A strengthened test that fails is a finding, never a task to make it pass. +Strengthen a self-derived test with unique kills by replacing its expected value with an independent one; if none exists, keep it with a `mutant` marker, as `test-oracle-reviewer` defines, naming each fault it alone catches in words, since mutant ids and line numbers change between runs. +A strengthened or gap test that fails is a finding, never a task to make it pass: report the test, the independent expected value and its source, and the code's actual output to whoever assigned the run in editing mode, or to the author in review mode, and change neither test nor code meanwhile, since whichever is edited first becomes the specification. Delete a self-derived test with no unique kills. A test of unknown oracle with unique kills has already passed the challenge; strengthen it. -Challenge a test of unknown oracle without unique kills. +Challenge a test of unknown oracle without unique kills, unless it is protected below, since its fate does not depend on the answer. +No `scaffold` marker outlives the pass: resolve each as `test-oracle-reviewer` requires, sending its questions to whoever assigned the run, or to the author in review mode. Deletion is a set decision, not a per-test verdict. Two tests that both catch mutant M each have zero unique kills, and deleting both loses M. Choose the deletion set so the caught mutants afterward equal the caught mutants before. The same rule applies to assertions: a self-derived one that never fails across its test's kills is a deletion candidate. -Never propose deleting a regression test tied to an issue or fuzz crash, a test of untrusted-input parsing or bounds checks, or a doctest; strengthen or merge those only. -Treat a suite that deliberately uses snapshots as a change detector: dedupe it, do not delete it. +Protected tests are never proposed for deletion: a regression test tied to an issue or fuzz crash, a test of untrusted-input parsing or bounds checks, and a doctest. +That holds whatever their marker, because a refactor of a parser is where a dropped bounds check goes unnoticed. +Treat a suite that deliberately uses snapshots as a change detector as protected: dedupe it, do not delete it. Report flaky tests separately; deleting one hides a bug. ## Challenge Give a fresh-context agent the test, the code under test, and the mutants the rest of the suite already catches, and ask for one mutant that this test catches and the others miss, as a patch only. Apply and run the patch yourself; arguments do not count. -Add any mutant it finds to the domain catalog regardless of the verdict. +Record any mutant it finds in the evidence regardless of the verdict, and propose a reusable one for `references/domain-mutants.md` in a separate pull request, since neither mode edits skills. If it finds none, ask the human who wrote or prompted the test for the source of its expected value, and accept a source you can check, never a confirmation. -Without one, delete the test, or hand a protected test to the file's CODEOWNERS. +Without one, delete the test. ## Consolidate -Keep rstest case names readable, with `#[case::name]` or enum-valued `#[values]`. -Use proptest only for a real property; an assertion that re-implements the unit is a self-derived test with random inputs. +Consolidate into the case and property forms `test-writer` gives for the language, naming every case, since the kill matrix reports each case by its name. +Use a property test only for a real property; an assertion that re-implements the unit is a self-derived test with random inputs. Round-trip passes symmetric bugs, so every codec module keeps spec or capture vectors. -Do not add a never-panics property where a fuzz target already covers it. +Do not add a never-panics or never-throws property where a fuzz target already covers it. ## Invariant and output The caught-mutant set never shrinks except through deletions a human approves in a pull request. -Rerun the caught set after every deletion set and every consolidation, verify by rerunning rather than reasoning, and revert on any loss. -Triage surviving mutants as equivalent, coverage gap, or bug lead; a mutant that survives because its code is unreachable is a bug lead. +Rerun the caught set after every deletion set and every consolidation, verify by rerunning rather than reasoning, and revert on any loss; compare runs made with identical tool settings over the same scope. +After ruling out the scoping errors and tool artifacts the reference names, triage each survivor as equivalent (no observable change), unconstrained (an observable change no independent requirement decides, such as a log message's wording), coverage gap (a change an independent requirement rejects), or bug lead (the original code looks wrong, or is unreachable in every supported configuration). +Close each coverage gap with a test at the seam `tdd` defines, marked with the requirement that rejects the change and written with `test-writer`; a survivor only a test of internals could catch is not a gap but equivalent or a bug lead, as above. +A suite of independent tests yields few deletions; a pass may end in consolidations and gap tests only, or in no change. -In crate mode, open separate pull requests for deletions, consolidations, and strengthenings, or separate commits when amending an open pull request; they are different reviews. -Put the oracle table, kill matrix, challenge results, survivors, and found bugs in a pull request comment; the pull request body is the squash commit. +In editing mode, commit deletions, consolidations, strengthenings, and gap tests separately, each as soon as it is verified, so an interrupted run loses little; they are different reviews. Change only tests and source comments; never modify non-test source otherwise. -In pull request mode, post one review instead: inline comments on the affected tests, with the tables in `<details>` blocks. -Deletions, consolidations, strengthenings, and small non-test changes that enable an independent oracle become requests to the author, with the current caught set as the bar. +In review mode, post one review: inline comments on the affected tests, with the tables in `<details>` blocks. +Deletions, consolidations, strengthenings, gap tests, and small non-test changes that enable an independent oracle become requests to the author, with the current caught set as the bar. diff --git a/.agents/skills/test-pruner/references/mutants.md b/.agents/skills/test-pruner/references/cargo-mutants.md similarity index 72% rename from .agents/skills/test-pruner/references/mutants.md rename to .agents/skills/test-pruner/references/cargo-mutants.md index 884e2ba2..bde8e62e 100644 --- a/.agents/skills/test-pruner/references/mutants.md +++ b/.agents/skills/test-pruner/references/cargo-mutants.md @@ -1,14 +1,12 @@ # cargo-mutants mechanics -cargo-mutants reports whether the suite caught each mutant. -It has no user-defined mutation operators and keeps per-test results only in its logs, so both are built on its outputs. +cargo-mutants keeps per-test results only in its logs, so the kill matrix is parsed from them. These notes target cargo-mutants 27.1. ## Run Scope with `--file '<package-dir>/src/<module>/**'` for a crate or module, `--re '<fn>|<fn>'` for a pull request's changed functions, or `--in-diff <diff>` for its whole diff; build time dominates, so scope first. A test-only diff yields no mutants under `--in-diff`, so scope with `--re` to the production functions the changed tests call, including unchanged ones. -Make no deletion decision on an empty or near-empty viable sample; with nothing to catch, every test has zero unique kills and the invariant passes trivially. Package directories vary; take the path from `cargo metadata` rather than assuming `crates/<name>`. ```sh @@ -56,49 +54,9 @@ $expected = Select-String -Path <clean-run-log> -Pattern '^test (\S+)(?: - shoul } ``` -A unique kill is a mutant caught by exactly one test in scope. For assertion-level evidence, read the `panicked at <file>:<line>` location of each failure; an assertion whose line never appears across its test's kills catches nothing the others miss. ## Rerun -Rerun with the same scope and flags after each change. Run the scoped tests on the clean tree first; with `--baseline skip`, a suite broken by a test edit marks every mutant caught. Any mutant that moved from caught to missed without an approved deletion explaining it means revert. - -## Domain mutants - -Write each as a small patch; apply it, run the scoped tests, revert it with `git apply -R`; caught means any test failed. -Patches are pull request evidence, not repository files. - -Codec catalog: - -- Swap endianness on one field read or write. -- Length prefix off by one in either direction. -- Remove one bounds check or remaining-length test. -- Wrong padding count, or padding read but not skipped. -- Swap two adjacent fields of the same type. -- Flip one bit in a flags field. -- Write one byte fewer than `size()` reports. -- Return `Ok` on a malformed-input branch that returns `Err`. -- Accept a reserved or out-of-range enum value. - -Async and resource catalog: - -- Remove or shorten a deadline. -- Detach a task instead of aborting it, or hold a `JoinHandle` where an abort-on-drop handle is required. -- Return early before a drain or flush. -- Swallow an error as `Ok`. -- Skip recording a state flag. -- Swap two status or close codes. - -Decision and validation catalog, for each security or policy decision: remove it, weaken it, key it on the wrong identity, or apply it to the wrong object. - -- Always return `Ok` from the check. -- Accept any candidate by dropping a filter. -- Look up by the wrong key, such as a name hash instead of a thumbprint. -- Stop after the first candidate. -- Ignore one input source, such as caller-provided roots. -- Skip verification of a candidate's chain. -- Swap error precedence or report the wrong error. - -Add every mutant the challenger produces that the rest of the suite misses. diff --git a/.agents/skills/test-pruner/references/domain-mutants.md b/.agents/skills/test-pruner/references/domain-mutants.md new file mode 100644 index 00000000..63a6abf1 --- /dev/null +++ b/.agents/skills/test-pruner/references/domain-mutants.md @@ -0,0 +1,37 @@ +# Domain mutants + +Neither mutation tool takes user-defined operators, so write each domain mutant as a patch. +Take it, or rebuild a challenger's patch, with `git diff --output=<file>` from an edit you then revert, since a hand-written hunk header is easily wrong and a Windows PowerShell redirect re-encodes the patch as UTF-16 with CRLF, which `git apply` rejects. +`git apply` it, run the scoped tests, and `git apply -R` it; caught means a test failed, and a patch that does not build proves nothing until it does. +Patches are pull request evidence, not repository files. + +Codec catalog: + +- Swap endianness on one field read or write. +- Length prefix off by one in either direction. +- Remove one bounds check or remaining-length test. +- Wrong padding count, or padding read but not skipped; accept non-zero pad bits. +- Swap two adjacent fields of the same type. +- Flip one bit in a flags field. +- Write one byte fewer than the size computation reports. +- Return success (`Ok`, `true`) on a malformed-input branch that returns an error or throws. +- Accept a reserved or out-of-range enum value. + +Async and resource catalog: + +- Remove or shorten a deadline. +- Detach a task instead of aborting it, or hold a handle that neither aborts on drop nor cancels on dispose where one is required. +- Return early before a drain or flush. +- Swallow an error as success. +- Skip recording a state flag. +- Swap two status or close codes. + +Decision and validation catalog, for each security or policy decision: remove it, weaken it, key it on the wrong identity, or apply it to the wrong object. + +- Always return success from the check. +- Accept any candidate by dropping a filter. +- Look up by the wrong key, such as a name hash instead of a thumbprint. +- Stop after the first candidate. +- Ignore one input source, such as caller-provided roots. +- Skip verification of a candidate's chain. +- Swap error precedence or report the wrong error. diff --git a/.agents/skills/test-pruner/references/stryker.md b/.agents/skills/test-pruner/references/stryker.md new file mode 100644 index 00000000..13bb6ef2 --- /dev/null +++ b/.agents/skills/test-pruner/references/stryker.md @@ -0,0 +1,69 @@ +# Stryker.NET mechanics + +Stryker.NET lists the tests that killed each mutant in its JSON report, but only when bail is disabled, so the kill matrix is read from the report. +These notes target Stryker.NET 5.0.0 with xUnit v2 on the VsTest runner. + +## Install + +In the solution directory, `dotnet new tool-manifest` then `dotnet tool install dotnet-stryker --version 5.0.0`; the manifest it writes is a repository file, not output, so leave it untracked unless the pull request adopts it. + +## Run + +Run the clean suite first with a TRX log, because the report names tests by id and the TRX carries the id with the display name: + +```powershell +dotnet test <TestProject>.csproj --logger "trx;LogFileName=clean.trx" --results-directory <scratch-dir>/trx +``` + +Confirm the TRX lists the tests you expect before trusting any result, because `dotnet test` passes when it discovers none, as with a wrong project or filter. + +Then, from the test project directory: + +```powershell +dotnet stryker --mutate "**/<Module>/*.cs" --disable-bail --break-on-initial-test-failure --reporter json --reporter cleartext --output <scratch-dir> --skip-version-check +``` + +`--mutate` takes several globs, `!<glob>` excludes, and `<file>.cs{<start>..<end>}` limits to a character span; Stryker mutates the whole project in memory before filtering, so compile-error warnings about files outside the scope are noise. +`--disable-bail` runs every covering test to completion; by default Stryker aborts a mutant's run at the first failure, and `killedBy` then holds only the tests that had failed by then, a partial list that makes kills look unique. +`--break-on-initial-test-failure` stops the run when the clean suite fails, which a test edit can cause, because a matrix built over failing tests proves nothing. +For a change's diff, `--since:<branch, tag, or full SHA>` selects its mutants; a revision expression such as `HEAD~1` or an abbreviated SHA is rejected. +A test-only diff still yields mutants under `--since`: every mutant covered by a test in a changed test file counts as changed. +Pass `--project <Name>.csproj` only when the test project references more than one project, and `--solution <path>.sln` when it targets .NET Framework, where Stryker requires it. +Coverage analysis, `perTest` by default, runs only the tests covering each mutant, so the matrix spans the whole test project without scoping tests and a unique kill is unique across the project; restricting to a test class needs `test-case-filter` in `stryker-config.json`, since the 5.0.0 command line rejects it as a flag. +Stryker builds the test project itself and writes a `.gitignore` of `*` into `--output`, so the output stays untracked wherever it is. +Build and the initial run take a few minutes before the first mutant, and a whole library of a few thousand lines can run for most of an hour, so scope first; timeouts are computed per mutant from the initial run, and `additional-timeout` and `timeout-ratio` are config-file options. + +## Kill matrix + +`<scratch-dir>/reports/mutation-report.json` holds `files[<path>].mutants[]` with `id`, `mutatorName`, `replacement`, `location`, `status` (`Killed`, `Survived`, `NoCoverage`, `Timeout`, `CompileError`, `Ignored`), and `coveredBy` and `killedBy` lists of test ids, and `testFiles[<path>].tests[]` with `id` and `name`. +`name` is the method's qualified name without arguments, so every row of a `[Theory]` shares one name; `id` is the VsTest test case id, which the TRX carries as `UnitTest/@id` with the display name and arguments. +Key on the id throughout: display names collide when xUnit truncates long string arguments. + +```powershell +$report = Get-Content -Raw <scratch-dir>/reports/mutation-report.json | ConvertFrom-Json +[xml]$trx = Get-Content -Raw <scratch-dir>/trx/clean.trx +$name = @{} +foreach ($t in $trx.TestRun.TestDefinitions.UnitTest) { $name[$t.id] = $t.name } +$report.files.PSObject.Properties | Where-Object Name -like '*<Module>*' | ForEach-Object { + foreach ($m in $_.Value.mutants | Where-Object status -eq 'Killed') { + foreach ($id in $m.killedBy) { "$($m.id)`tL$($m.location.start.line) $($m.mutatorName): $($m.replacement)`t$id`t$($name[$id])" } + } +} +``` + +Read kills only from `killedBy`; `coveredBy` lists the tests that ran against a mutant, not those that failed. +The script lists `Killed` mutants only; attribute a `Timeout` by applying it by hand, as below, and running its `coveredBy` tests one at a time before counting it caught. +One row with an argument xUnit cannot serialize makes the whole theory one test id, so the matrix cannot tell its rows apart; `test-writer`'s C# forms avoid this. +Stryker keeps no per-test output, so for assertion-level evidence apply the mutant by hand from `location` and `replacement`, run the killing test with `dotnet test --filter "FullyQualifiedName~<Class>.<Method>"`, and read the failing assertion from the stack trace. +Mutant ids are assigned in order over the whole project, so an edit anywhere can renumber them, and lines shift with edits above; across runs match mutants by file, `mutatorName`, `replacement`, and the text of the mutated line in `files[<path>].source`, and mutants sharing all four by their order in the file, which test and comment edits leave unchanged. + +## Rerun + +Refresh the clean run and its TRX before each rerun. +For a change's diff, measure and rerun with `--mutate` over the files a `--since` run mutated, never comparing `--since` runs: a test edit changes which mutants `--since` selects, so a lost kill drops out instead of surviving. +Do not use `--with-baseline`; it reuses results for mutants it considers unchanged, the opposite of a rerun. +Identical settings include `mutation-level` and any `test-case-filter`, since either changes the mutant population or the tests that can kill it. + +## Pitfalls + +Verify a surprising status by applying the mutant by hand: Stryker can report as survived a mutant in a static initializer, whose value is computed before the mutant switches on, or a replacement that would not compile on its own, and a machine at full load turns slow kills into timeouts. diff --git a/.agents/skills/test-reviewer/SKILL.md b/.agents/skills/test-reviewer/SKILL.md new file mode 100644 index 00000000..3b45616e --- /dev/null +++ b/.agents/skills/test-reviewer/SKILL.md @@ -0,0 +1,15 @@ +--- +name: test-reviewer +description: Review changed Rust or C# tests against the `test-writer` rules, including a named bug, an independent oracle, non-branching parametrized bodies, checked snapshots, and real properties. Use when a diff or pull request adds or changes tests, or when asked to review test organization or parametrization. +--- + +# Test reviewer + +Invoke `test-writer` and use its rules as review criteria without following its generation workflow. +Review only changed tests unless the user requests a broader audit. +Run `test-oracle-reviewer` on each changed test; this skill covers a test's shape, not where its expected values come from. +When a test's name and assertions do not say which bug it catches, ask the author rather than guess. +When the change touches an invariant from an applicable intent or contract file that no test checks, ask what pins it and whether a property test could exercise it across generated inputs. +A new or changed snapshot is a behavior claim; check that the pull request intends it and that the value is right. +Report each violation with its exact location and a concrete correction, and group repeated instances that share one correction. +Return no findings when the changed tests already comply. diff --git a/.agents/skills/test-writer/SKILL.md b/.agents/skills/test-writer/SKILL.md new file mode 100644 index 00000000..5d068d2f --- /dev/null +++ b/.agents/skills/test-writer/SKILL.md @@ -0,0 +1,35 @@ +--- +name: test-writer +description: Design tests that catch bugs, choosing between an example test, parametrized cases, a snapshot, a property, and a fuzz target, in Rust (`rstest`, `expect-test`, `proptest`) or C# (xUnit `[Theory]`, CsCheck). Use whenever adding or changing unit, integration, snapshot, or property tests in Rust or C#. +--- + +# Test writer + +First name the bug the test catches that existing tests miss; without one it costs runtime and maintenance and guards nothing. +Take the expected value from a source independent of the code under test, as `test-oracle-reviewer` defines; a value derived from the implementation detects change, not bugs. +Mark that source as `test-oracle-reviewer` defines, and assert only what it decides, because an assertion past it pins how the code works today; add one only for feedback while implementing, marked `scaffold`, and never in place of a decision `tdd` says to ask for. +Test at the seam `tdd` defines, the public boundary where the behavior is observed. +A test that fails while the code still does what its marked source requires asserts past that source: narrow it to what the source decides, or delete it unless `test-pruner` protects it; a snapshot that fails only on representation is re-read and updated instead. +Reach nothing beyond loopback, because an external host makes a failure unreproducible. +Each rule here takes a form per language: `references/rust.md` for Rust, `references/csharp.md` for C#; load only the language in scope. + +## Properties + +Look for a property before writing example tests, because a property holds across generated inputs while examples show only the cases someone thought of. +Candidates are a round-trip through encode and decode, a size computation matching the bytes written, and an invariant from the applicable intent or contract file that speaks of inputs or state, such as one that must survive any sequence of operations. +Reuse the existing generator for each type and add new ones beside it, so each type's generator is written once. +With no property that must hold across many inputs, write example tests; a property whose assertion re-implements the code under test checks the code against itself. +A parser of untrusted input gets a fuzz target rather than a never-panics property, because the fuzzer is coverage-guided and keeps a corpus, which a few hundred random cases are not; where no fuzz harness exists to add one to, a property over malformed inputs stands in, and a pull request comment says the parser is not fuzzed. + +## Snapshots + +Snapshot structured values such as a parsed config or a decoded message, with the expected value beside the assertion. +Taking or updating a snapshot is the oracle, so read and check it, because its value is whatever the code produced; when a spec or capture defines the value, assert that instead. +Move a snapshot or blob longer than a screen into a sibling file, so the source shows the test rather than its data; keep short byte vectors inline so each field can carry its comment. + +## Examples and cases + +Parametrize only when every case checks the same behavior with different data, and name each case so a failure names the case, not an index. +A body that branches on its parameters, through a conditional or an optional expected value, is several tests behind one name, so a failure reports a case instead of the behavior that broke; write one test per behavior and share setup through a helper. +A cross product of parameters is right only when every combination is a real scenario; one that needs a conditional to skip or adjust a combination is that branching. +Keep each regression test standalone and named for its issue, because its setup reproduces the bug's exact conditions and a case table would generalize them away. diff --git a/.agents/skills/test-writer/references/csharp.md b/.agents/skills/test-writer/references/csharp.md new file mode 100644 index 00000000..3d19522d --- /dev/null +++ b/.agents/skills/test-writer/references/csharp.md @@ -0,0 +1,16 @@ +# C# forms + +The repository's C# tests are xUnit v2; each rule in `SKILL.md` takes these forms there. + +Properties use CsCheck: compose `Gen<T>` with LINQ and `Sample` it; each type's generator lives in one static class of the test project. +CsCheck rather than FsCheck, because its generators shrink without per-type shrinkers, so a composed generator costs one declaration, and it adds no FSharp.Core to a C# test project. +Releases after CsCheck 2.14.1 need .NET 6 or later, so a .NET Framework test project pins 2.14.1, the last on .NET Standard 2.0. +A failing `Sample` prints a seed; replay it with `seed:` or the `CsCheck_Seed` environment variable, and pin the failure by keeping the shrunk value in a regression test rather than the seed, since a generator change moves the seed's meaning. + +Snapshots are written by hand: the expected value inline or, past a screen, in a sibling file copied into the test output. +Nothing rewrites a stale value, so edit each by hand after reading it. + +Cases are `[Theory]` with `[InlineData]` rows for literal data, whose display name carries the arguments, so a failure names the row. +Other data goes through `[MemberData(nameof(<Member>))]` whose rows hold only values xUnit can serialize, such as a string key the test resolves to its data, because one row it cannot serialize turns the whole theory into one test case that no filter or mutation report can split. +A cross product is a `[MemberData]` member that yields it. +A regression test is a separate `[Fact]`. diff --git a/.agents/skills/test-writer/references/rust.md b/.agents/skills/test-writer/references/rust.md new file mode 100644 index 00000000..d259c418 --- /dev/null +++ b/.agents/skills/test-writer/references/rust.md @@ -0,0 +1,14 @@ +# Rust forms + +Each rule in `SKILL.md` takes these forms in Rust. + +In a doctest, write the marker as a hidden line, `# // oracle: ...`, so it stays out of the rendered documentation. + +Properties use `proptest`, whose generators are the crate's strategies. + +Snapshots use `expect-test`, whose `UPDATE_EXPECT=1` rewrites every stale value after a representation change. +Use `expect_file!` for a snapshot in a sibling file and `include_bytes!` for a blob. + +Cases are `rstest`, each named with `#[case::name]`. +A cross product is enum-valued `#[values]`, so each combination's name says what it checks. +A regression test is not an `rstest` case. diff --git a/.agents/skills/variants/SKILL.md b/.agents/skills/variants/SKILL.md new file mode 100644 index 00000000..657d2390 --- /dev/null +++ b/.agents/skills/variants/SKILL.md @@ -0,0 +1,57 @@ +--- +name: variants +description: Run two or three isolated probe implementations of one brief that differ in named decisions or approaches, judge them blind against fixed criteria, and return a decision brief from which the human or the lead picks or composes the final design. Use inside a delivery or intent flow once the alternatives are named and only implementation evidence can settle them, or when the user asks to try several approaches. +--- + +# Variants + +A variant is a probe: build it only far enough to measure the criteria and never merge it, because probe code carries shortcuts and has not been through the review loop; the chosen design - one variant, or the best decisions of several - is implemented fresh and the criteria rerun on it. +A run answers a question nobody could settle on paper; when the facts or the human can settle it, ask instead, since three probes cost more than one question. + +## Two modes + +- Tournament: intent and contract are fixed and the candidates are approaches; whoever runs it picks, within autonomy. +- Experiment: the candidates are decisions the human has not committed; the human picks and writes the commitment, so no intent file changes until they do. + +Never run a candidate that contradicts adopted intent; that is drift with a budget, not an experiment. + +## Compose + +Candidates come from alternatives the human voiced, the options of an escalation, or alternatives you derive and name before launch; without a named difference there is nothing to run. +Vary one decision when the open decisions are independent; when they are coupled, make each variant a coherent design - a full set of answers - and compare designs, because a bundle that mixes answers at random tests nothing. +Three variants by default, raised only by the human, because each costs a build. +Fix the criteria before the first variant starts, choosing from `references/criteria.md`, each with how it is measured; the acceptance checks are written then and shared, since a check a variant writes for itself proves only that it did what it did. +When no criterion can distinguish the candidates, return the question to the human rather than inventing a proxy. + +## Run + +Same base commit, same brief apart from the named differences, same model for every variant; these reduce what else could explain a difference, without proving the design caused it. +Each variant in its own worktree or session, never shown another's work, or they converge. +The contract and any blind suite stay read-only in every variant, and a defect found in them is escalated outside the run, because a candidate that moves its oracle is not comparable. +A variant stops once every criterion can be measured on it, or earlier when its candidate proves infeasible, since the run has answered for it; one that fails a must-pass criterion is disqualified from winning but keeps its measurements, since they still inform a composed design, and when all fail, investigate before concluding anything from the run. + +## Judge + +A fresh context, from a different model family when available, gets the criteria, the measurements, and snapshots labelled A, B, C stripped of branch and commit metadata; never who or which model built which, the workers' transcripts or rationales, or your preference or proposed design, because each of those carries the framing the blind read is meant to remove. +It ranks per criterion with the evidence it turned on, names an attribution to one decision only where the evidence isolates it, and reports bundled effects as unidentifiable otherwise. +A variant another beats on every criterion that splits them is dropped, so the brief carries only the front where a trade exists; a criterion that split nothing is dropped from the brief too. +"No clear winner" is a verdict; so is "none of these", with the decisions a better design would combine. + +## Brief and after + +Write the decision brief from the template as an artifact, never committed; it is what the human reads instead of three implementations. +Keep the variant branches until the design they informed has merged, since the final implementer may read how a probe handled a decision; then delete them, because the brief is what survives. +In an experiment, proceed on the proposed design while the human is away only when the verdict is clear by the criteria, logged as provisional; the human adopts or reverses from the brief, at the latest when they merge, since nothing resting on a provisional decision merges without them. +A verdict that is not clear stops this thread and waits; other threads continue. + +```md +# Experiment <n>: <question> +Base: <commit>. Variants: A <decisions>; B <decisions>; C <decisions>. +Criteria: <each with how it was measured; must-pass marked>. +| Criterion | A | B | C | +Verdict (blind): <ranking; evidence; attributions the evidence isolates; effects left unidentifiable>. +Per variant: <what it demands of callers; what it forecloses and the cost of switching later; surprises met>. +Proposed design: <a variant, or the decisions combined>, because <evidence>. +Status: proceeding provisionally | waiting. +To adopt or rewrite: <one candidate commitment sentence per decision>. +``` diff --git a/.agents/skills/variants/references/criteria.md b/.agents/skills/variants/references/criteria.md new file mode 100644 index 00000000..d56f21cc --- /dev/null +++ b/.agents/skills/variants/references/criteria.md @@ -0,0 +1,39 @@ +# Criteria + +A criterion measures something the design commits to and cannot polish away later; anything a lint catches or the final implementation fixes in a line - panics, licences, formatting - measures nothing about the design and is not a criterion. +Measure with the tool, never by estimate, and the same tool on every variant, or the numbers do not compare. + +## Must-pass + +A variant that fails one is disqualified from winning; its measurements stay in the brief. + +- The acceptance checks, written before the variants and shared by all, pass in full; a count of passes is not a rank, and a test a variant wrote for itself does not count. +- Adopted intent and the contract hold, clause by clause, judged on the probe. +- In an experiment, the decision under test was exercised, not routed around; a probe that makes the decision irrelevant has answered nothing, however clean. + +## Default ranking set + +| Criterion | Measured by | Trap | +|---|---|---| +| Code size | Non-test lines and new items (types, traits, modules); lines changed in existing code against lines added | Smaller by omission scores well; the gate guards only what the checks exercise | +| API surface | Public items at the seam, their parameters, generics, and lifetimes; the caller for the common case, written out | Count what a caller sees, not what exists | +| Self-containment | Existing items the new code references, excluding std and its own; definitions the reviewer had to open to follow it | Self-contained by duplication: a reimplemented helper scores well unless the reimplementation is recorded against it | +| Reviewability | Whether the reviewer can reconstruct the change from the diff alone and it matches the stated goal; files in the diff | Finding counts penalise novelty, so the most different variant loses by construction | + +Self-containment is the inward dependency, what a reader must already know; locality below is the outward footprint, what the change touches; a variant can score well on one and badly on the other. + +## When the intent asks + +| Criterion | Use when | Measured by | Trap | +|---|---|---|---| +| Extensibility | The intent's future names a direction | Sketch that next change on each probe; files and lines touched | A future you invent measures your preference | +| Locality | The component has neighbours | Files and modules touched outside the component; dependency edges added | - | +| Dependencies | A variant adds a crate | Transitive crates not already in the tree; features enabled; duplicate versions | - | +| Performance | The intent or contract states a bound or names a direction | Margin on a fixed workload; allocations; compile time; binary size | A workload chosen after seeing the variants | +| Test-only access | Behaviour may be hard to observe | `#[cfg(test)]` items, widened visibility, `#[doc(hidden)]`, mocks of internal traits added to the component; fewer is better | Zero by not testing scores the same; the gate is what stops it | +| Failure handling | The contract names failure cases | Cases a caller can distinguish at the seam, the match arm written out; whether an error carries what locates the cause | Panic counts belong to the lint, not here | +| Compatibility | Existing callers, config, or wire format | Call sites changed; migration needed or not | - | + +Three to five criteria, each able to split the candidates. +A criterion stays in the brief only if it splits them and measures what its name says; a metric that reads worse as the design improves - setup lines rising because the implementation stopped leaking internals - is mislabelled, not a trade-off for the human to weigh. +No weights: report per criterion, and let the proposed design state the trade it makes, because weighting is the human's judgement.