From c19beb3e93441c1d25b7a1ae5f43483b3947cc55 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Sat, 31 Oct 2026 23:06:43 -0700 Subject: [PATCH 01/13] Add shared skills for agents working in the repo All of Modelplane's code is written by agents, and each maintainer's agent loads its own personal skills. The conventions we expect, such as how to factor a composition function, shape an XRD or write a design doc, live in those personal skills rather than in the repo, so each agent knows a different subset. This adds my skills for writing code, Python composition functions, KRM APIs, design docs, prose and Nix to .agents/skills, where opencode and other agent harnesses load them for anyone working in the repo. They're adapted to Modelplane: the composition function skill describes how Modelplane builds and tests functions, the testing advice defers to CONTRIBUTING.md, the XRD skill covers YAML schemas rather than Go types, and the writing skills describe the project's style rather than mine. Agents imitate the prose they're given, so a new agents-vale flake check lints the skills with the same ai-tells rules the docs use. Signed-off-by: Nic Cope --- .../crossplane-python-functions/SKILL.md | 295 ++++++++++++ .agents/skills/design-docs/SKILL.md | 185 ++++++++ .../skills/engineering-principles/SKILL.md | 272 +++++++++++ .agents/skills/krm-api-design/SKILL.md | 284 ++++++++++++ .agents/skills/nix-flake/SKILL.md | 109 +++++ .agents/skills/nix-tools/SKILL.md | 72 +++ .agents/skills/python-code-factoring/SKILL.md | 215 +++++++++ .../references/example.md | 125 +++++ .agents/skills/writing-style/SKILL.md | 438 ++++++++++++++++++ .../references/de-slop-review.md | 141 ++++++ .../skills/writing-style/references/voice.md | 254 ++++++++++ .opencode/vale/.gitignore | 1 + .opencode/vale/.vale.ini | 19 + nix/checks.nix | 23 + nix/docs.nix | 3 + 15 files changed, 2436 insertions(+) create mode 100644 .agents/skills/crossplane-python-functions/SKILL.md create mode 100644 .agents/skills/design-docs/SKILL.md create mode 100644 .agents/skills/engineering-principles/SKILL.md create mode 100644 .agents/skills/krm-api-design/SKILL.md create mode 100644 .agents/skills/nix-flake/SKILL.md create mode 100644 .agents/skills/nix-tools/SKILL.md create mode 100644 .agents/skills/python-code-factoring/SKILL.md create mode 100644 .agents/skills/python-code-factoring/references/example.md create mode 100644 .agents/skills/writing-style/SKILL.md create mode 100644 .agents/skills/writing-style/references/de-slop-review.md create mode 100644 .agents/skills/writing-style/references/voice.md create mode 100644 .opencode/vale/.gitignore create mode 100644 .opencode/vale/.vale.ini diff --git a/.agents/skills/crossplane-python-functions/SKILL.md b/.agents/skills/crossplane-python-functions/SKILL.md new file mode 100644 index 000000000..0fe6e7dba --- /dev/null +++ b/.agents/skills/crossplane-python-functions/SKILL.md @@ -0,0 +1,295 @@ +--- +name: crossplane-python-functions +description: Write and review Modelplane's Python composition functions under functions/. Use when writing, debugging, refactoring or reviewing a composition function, or when dealing with required resources, readiness, resource gating, deletion ordering or Usages. +--- + +# Writing Crossplane Python Composition Functions + +## How Modelplane Builds Functions + +The Working on composition functions section of CONTRIBUTING.md is the +reference for how Modelplane lays out, builds and tests a function. In short: + +- Each function is a standalone Python package under `functions//`. Its + logic is in `function/fn.py`, as a `Composer` that a gRPC `FunctionRunner` + calls. `compose-inference-gateway/function/fn.py` is the model to follow. +- `FunctionRunner.RunFunction` builds the response with `response.to(req)`, + runs the `Composer`, and returns the response. That's the SDK's standard + runner, and it's correct here. +- Functions import generated Pydantic models from the `models` package, + generated into `schemas/python/` from the XRDs and dependency CRDs. Managed + resources use the `.m.` (namespaced) variants: + +```python +from models.ai.modelplane.inferencegateway import v1alpha1 +from models.io.crossplane.m.helm.release import v1beta1 as helmv1beta1 +``` + +- `nix run .#build` regenerates the models after an XRD or dependency + changes. Never edit them by hand. +- `nix run .#test -- ` runs one function's tests, and + `nix flake check` runs everything. `nix run .#run` deploys the project to a + local control plane. + +The rest of this skill covers how Crossplane behaves, which is what most bugs +in a function get wrong. + +## Composing Resources + +Use Pydantic models for everything with a generated model. They catch field +name mistakes at write time: + +```python +resource.update( + rsp.desired.resources["network"], + networkv1beta1.Network( + spec=networkv1beta1.Spec( + forProvider=networkv1beta1.ForProvider( + project="my-project", + autoCreateSubnetworks=False, + ), + ), + ), +) +``` + +Raw dicts are the fallback for CRDs without generated models (Gateway API, +MetalLB, Usages, etc.): + +```python +resource.update(rsp.desired.resources["gateway-class"], { + "apiVersion": "gateway.networking.k8s.io/v1", + "kind": "GatewayClass", + "metadata": {"name": "envoy"}, + "spec": {"controllerName": "gateway.envoyproxy.io/gatewayclass-controller"}, +}) +``` + +### Protobuf Number Coercion + +All numbers from XRD fields arrive as Python `float`, not `int`. Cast +explicitly: + +```python +desired_envs = int(xr.spec.environments) # protobuf delivers as float +``` + +## Readiness + +Set XR readiness via `rsp.desired.composite.ready`: + +```python +rsp.desired.composite.ready = fnv1.READY_TRUE +``` + +Do NOT set the `Ready` condition via `fnv1.Condition`, because it's reserved. +Do NOT use `TARGET_COMPOSITE_AND_CLAIM`, because Crossplane v2 has no claims. + +For composed resources, check the appropriate condition (not always `Ready`): + +```python +from crossplane.function import resource + +def _has_condition(req, name, cond): + """Check if an observed composed resource has a condition set to True.""" + observed = req.observed.resources.get(name) + if observed is None: + return False + return resource.get_condition(observed.resource, cond).status == "True" + +# Crossplane MRs use Ready +if _has_condition(req, "my-cluster", "Ready"): + rsp.desired.resources["my-cluster"].ready = fnv1.READY_TRUE + +# Gateway API uses Accepted +if _has_condition(req, "my-gateway", "Accepted"): + rsp.desired.resources["my-gateway"].ready = fnv1.READY_TRUE +``` + +Mark resources without conditions (ProviderConfigs, ConfigMaps) as +always-ready: + +```python +rsp.desired.resources["my-providerconfig"].ready = fnv1.READY_TRUE +``` + +## Results as Kubernetes Events + +`response.normal()` and `response.warning()` emit Kubernetes events visible +in `kubectl describe`. Use them deliberately: + +- **Normal (every reconcile, time-bounded):** "Waiting for: gke-cluster". + Expected, self-resolving. Stops when the resource becomes ready. +- **Normal (transition, once):** "GKE cluster ready, composing KServeStack". + A milestone. Detect transitions by comparing current state to observed state. + +- **Warning (every reconcile, persistent):** "spec.kserve.cluster.gke is + required". + + The user can fix it, and it won't self-resolve. + +Detect transitions: +```python +was_ready = resource.get_condition( + req.observed.composite.resource, "Ready" +).status == "True" + +if not not_ready: + rsp.desired.composite.ready = fnv1.READY_TRUE + if not was_ready: + response.normal(rsp, "Ready, gateway: 34.55.233.135") +else: + response.normal(rsp, f"Waiting for: {', '.join(not_ready)}") +``` + +Never emit Normal events on every reconcile in the steady state. They +accumulate as Kubernetes Event objects. + +## Gating Resources on Dependencies + +Only compose a resource once Crossplane observes its dependencies. But once +composed, always keep emitting it. If a function omits a resource from desired +state, Crossplane deletes it. + +```python +pc_observed = "provider-config" in req.observed.resources +release_exists = "cert-manager" in req.observed.resources + +# Gate on dependency, but always emit once it exists +if pc_observed or release_exists: + resource.update(rsp.desired.resources["cert-manager"], ...) +``` + +For readiness-based gating (e.g., KServe waits for cert-manager): + +```python +cert_ready = _has_condition(req, "cert-manager", "Ready") +kserve_exists = "kserve" in req.observed.resources + +if cert_ready or kserve_exists: + resource.update(rsp.desired.resources["kserve"], ...) +``` + +## Required Resources (Cross-XR Reads) + +Use required resources to read resources the function doesn't own: + +```python +from crossplane.function import request, response + +response.require_resources( + rsp, name="cluster", + api_version="platform.example.org/v1alpha1", + kind="Cluster", + match_name="my-cluster", +) + +cluster = request.get_required_resource(req, "cluster") +if cluster is None: + return # Crossplane will re-call with the resource +``` + +Required resources are dicts, not Pydantic models. They're external resources +that Crossplane resolves. + +The empty `match_labels={}` selector doesn't work. Protobuf serialization loses +the empty map, causing the `oneof match` to be unset. Use a real label to match +on, or use `match_name` for specific resources. + +## Deletion Ordering with Usages + +When you delete a composite, all composed resources get deletion timestamps +simultaneously. Resources with implicit dependencies (ProviderConfigs used by +Helm releases, clusters used by stacks installed on them) need explicit ordering. + +Use `protection.crossplane.io/v1beta1` Usages: + +```python +resource.update(rsp.desired.resources["usage-pc-by-release"], { + "apiVersion": "protection.crossplane.io/v1beta1", + "kind": "Usage", + "metadata": {"namespace": "my-namespace"}, + "spec": { + "of": { + "apiVersion": "helm.m.crossplane.io/v1beta1", + "kind": "ProviderConfig", + "resourceRef": {"name": "my-provider-config"}, + }, + "by": { + "apiVersion": "helm.m.crossplane.io/v1beta1", + "kind": "Release", + "resourceSelector": { + "matchControllerRef": True, + "matchLabels": {"my-label": "my-release"}, + }, + }, + "replayDeletion": True, + }, +}) +``` + +Key rules: + +- **One Usage per protected-by pair.** The selector resolves to the first + matching resource and stops. A pair of releases needs a separate Usage for + each, with distinct labels. +- **Use labels to distinguish resources** of the same kind in the selector. +- **`replayDeletion: True`** means that deleting the "by" resource also + deletes the Usage, and Crossplane then retries deleting the "of" resource. +- **Usages don't prevent Terminating state.** A namespace with a + `deletionTimestamp` blocks new object creation even if a Usage prevents its + final deletion. Use shared long-lived namespaces instead of per-resource + namespaces. + +## managementPolicies for Undeletable Resources + +Some resources can't be cleanly deleted (webhook-protected CRDs, resources with +controller-managed finalizers). Use `managementPolicies` to skip deletion: + +```python +release.spec.managementPolicies = ["Create", "Observe", "Update"] +``` + +This is appropriate when the underlying infrastructure is going away anyway +(e.g., KServe on a GKE cluster that you're deprovisioning). Crossplane orphans +the resource, and it goes away with the cluster. + +## Cluster-Scoped XRs Composing Namespaced Resources + +Cluster-scoped XRs don't auto-populate `metadata.namespace` on composed +namespaced resources. Set it explicitly: + +```python +helmv1beta1.Release( + metadata=metav1.ObjectMeta(namespace="my-namespace"), + spec=..., +) +``` + +Without this, the resource has no namespace in the XR's `resourceRefs` and +Crossplane can't GET it. + +## Debugging + +When something is slow or stuck during deletion, describe the resources: + +```bash +kubectl describe -n +``` + +Every "slow" deletion has a specific stuck resource with a specific error. +Common causes: + +- **ProviderConfig not found:** deleted before the resources that use it. Fix + with a Usage. +- **Namespace is terminating:** blocks ProviderConfigUsage creation. Fix by + using a shared namespace. +- **Webhook has no endpoints:** something deleted the webhook server pod before + the webhook-protected resources. Fix with `managementPolicies`. +- **Finalizer blocking deletion:** a remote controller hasn't processed the + deletion yet (e.g., GatewayClass `gateway-exists-finalizer`). Fix with + `managementPolicies` or a Usage to ensure Crossplane deletes the dependent + first. + +Never increase timeouts without first describing the stuck resources. The fix +is always in the deletion ordering, not the timeout. diff --git a/.agents/skills/design-docs/SKILL.md b/.agents/skills/design-docs/SKILL.md new file mode 100644 index 000000000..dd60e618a --- /dev/null +++ b/.agents/skills/design-docs/SKILL.md @@ -0,0 +1,185 @@ +--- +name: design-docs +description: Write and review Modelplane design documents - proposals and designs under design/. Use when drafting, authoring, or structuring a design doc, and when reviewing, refactoring, tightening, or cleaning up someone else's design doc, especially an AI-written one that reads poorly. Also use when asked whether a design doc is good, or to bring one up to the standard of a gold-standard example. +--- + +# Design Docs + +A design doc exists to win agreement on a problem and a proposed solution. Its +readers are peer maintainers who know the project the doc concerns. They often +will not know the problem itself, nor the technical area around it. That area +might be anything from a serving engine to a wire protocol. A good design doc +gives that reader the problem, the case that the proposal solves it, and the +alternatives it beat. They should finish it ready to approve the direction or +push back on it. + +Everything here serves that reader. A rule that rests only on convention or an +author's taste is not in this skill. + +## When to Use This (vs other skills) + +Use **this skill** for the structure and content of a design doc: whether it +frames its problem and argues its solution, and how well it handles its +alternatives. It +applies both to writing one from scratch and to fixing a poor one. + +Use **writing-style** for sentence-level voice and the AI-tell linting pass. A +doc that fixes its structure can still read AI, so clear the writing-style Vale +and de-slop pass before calling the doc done. + +Modelplane's design docs live under `design/`, and `design/design.md` is a +good example of the standard below. + +## The Standard + +These describe a good design doc. They also serve as the rubric to score one +against, and the checklist to refactor one toward. + +1. **Leads with the problem.** The reader reaches the proposal already + understanding the problem. A proposal makes sense only against a problem the + reader already feels. Open with the solution and they read the rest working + out what it is for. + +2. **Is self-contained.** Everything the argument rests on is in the doc. Links, + in-repo or out, are optional depth the reader may follow but never has to. A + design that makes sense only alongside issues, PRs or review threads has put + its content where the reader may never go. + +3. **Covers the domain, assumes the project.** Write for a maintainer who knows + the project but not the problem space. Explain the one property the proposal + rests on, and no more. A peer new to the area cannot buy into a problem + stated in its terms, and spelling out what a maintainer already knows wastes + them. + +4. **Reads as the end state, in the present tense.** Describe the design as the + first and only position, not the latest of several. Document status (`draft`, + `accepted`) is fine. A record of how the code got here is not: no "proven but + unmerged", no proof-of-concept branch name, no "this replaces an earlier + approach". That history makes the reader reconstruct the past instead of + reading the design. Discarded thinking goes to Alternatives, gathered there + rather than scattered inline. + +5. **Mechanics in broad strokes, API and UX in full.** Show the interface a user + or caller touches. That covers the YAML they write, the API objects they see, + the command they run, and the response they get. Show it with real, whole + examples. Keep the internals of a controller, function, or pipeline out. The + doc drives direction and buy-in, and internals only distract from that. They + belong in the repo, where they change with the code. + +6. **Proportion.** A section's size matches its importance to the whole doc, not + how recently it came up or how much the author enjoyed it. Readers weigh a + topic by the room it gets. An oversized section misleads them. A background + or future-work section much longer than the proposal points to a problem to + fix. + +7. **Takes a position on everything it records.** Give the reviewer something + concrete to approve or push back on. A point important enough to record at + design-doc level gets a position. Leave out a point the team could approve + and settle later. Do not add an Open Questions section, which an LLM above + all will fill with invented questions, and do not pose questions to the + reader. When a choice is close, pick one and put the runner-up in + Alternatives. + +8. **Alternatives honest and in their section.** Each real alternative, the + current system included, gets a fair account. Say what it is and what is good + about it, then why the proposal is better anyway. No strawmen, and no arguing + an objection inline where it breaks the thread. A reader who wonders "why not + X?" should find X in one place. + +9. **Examples real and whole.** Reuse a tested artifact, show the whole object + rather than a fragment, and carry one example through the doc. An example + does more for the reader than the prose around it, and a fabricated one + misleads because its detail invites trust. When you cannot build a credible + one, say so rather than invent. + +10. **As short as the three jobs allow.** Frame the problem, argue the solution, + cover the alternatives, then stop. Remove any paragraph the reader does not + need. Length costs the reader time, and it hides the argument. A design for + one feature that runs past roughly twice the length of a tight peer design + is a finding. Either it covers a large domain, or it is over-produced, so + justify it or cut it. + +## Authoring + +Before writing, settle who the doc is for and what decision it serves. Identify +the problem space the reader will not know, and state what falls outside the +doc's scope. Ask rather than guess. + +Then follow the skeleton. Most docs use some of it. Drop any section with +nothing to say. + +1. **Title.** Short and descriptive. Derive it again at the end from what the + doc argues. +2. **Status, date, author.** `draft` or `accepted`, the date, and who wrote it. + Use "I" for one author, "we" for co-authors. +3. **Summary** (longer docs). A few paragraphs that give the whole picture, so a + busy reader can stop there with the problem and the proposal clear. Put the + primary example here. +4. **Background.** The problem, what stands today, and the prior art. Here the + doc leads with the problem (standard 1) and covers the domain (standard 3). +5. **Goals.** What the doc delivers, and what it leaves out. A few bullets, + stated positively. +6. **Proposal.** The concrete design. Show the API and UX in full (standard 5), + with a worked example per case, including the awkward one. +7. **Future improvements** (optional). Out of scope now, plausible later. All + speculative material belongs here, and stays proportional (standard 6). +8. **Alternatives considered.** What else you weighed, and why it lost (standard + 8). + +Write the background as the story of how we got here and why the status quo fits +poorly now. The reader should reach the proposal already feeling the problem. +Cover only the property the proposal rests on. Respect what came before: state +the shortcoming and the new constraint that exposes it, without calling the old +system bad. + +Then cut and de-slop. A first draft runs long. Cut once for what the reader can +reconstruct, then again for what remains. Trade paragraphs of prose for a worked +example wherever one carries the point. Run vale over the whole doc and a +fresh-context de-slop review, and fix both until they come back clean. + +## Refactoring + +Refactoring a poor doc, often an AI-written one, is surgery on the document's +shape more than a copy edit. Read it against the standard before touching it, +and write the diagnosis down: for each point, whether it holds and where it +fails. That record is the before-state, and the feedback for the author. A fresh +reader catches what the author and the drafting agent cannot, so run the +diagnosis from a fresh context, or have another agent do it, briefed with the +standard and one strong example. + +Fix the structure before the prose, in this order, because fixing a sentence you +later cut wastes effort: + +1. **Buried or missing problem** (standard 1). Lift the sharp problem statement + into the opening of the background. If it sits only in the PR description, + move it into the doc. +2. **Internals where API and UX belong** (standard 5). Cut controller and + pipeline internals to broad strokes, and show the interface in full. Usually + the largest cut. +3. **Leaked chronology** (standard 4). Rewrite to the present-tense end state, + and move a discarded design to Alternatives. +4. **Off proportion** (standard 6). Cut the oversized section, then its + neighbours. +5. **The doc argues with itself or a reviewer** (standards 4 and 8). Move the + substance to Alternatives and rewrite the section positively, from first + principles. +6. **Open questions or hedged non-decisions** (standard 7). Make the call, or + cut the point if it does not warrant one. +7. **Thin or unfair alternatives** (standard 8). Give each a fair account and an + honest reason it lost. +8. **Fabricated or fragmentary examples** (standard 9). Replace with a real, + whole object. + +Prefer surgery to a rewrite. Keep the author's structure and content and move it +where you can. The author should still recognise the doc. A section built on a +framing error usually needs redoing from first principles, because patched prose +drifts into incoherence. + +Then de-slop, and do not skip it. Run vale and a fresh-context de-slop review, +and loop until both come back clean. A doc that meets the standard but reads as +though an LLM wrote it is not done. + +Prove it improved. Score the refactor against the standard again, blind where +you can. Give a fresh agent the rubric and a strong example without saying which +version is which. The refactor succeeds when it scores at or above the exemplar +on the same rubric the diagnosis used. diff --git a/.agents/skills/engineering-principles/SKILL.md b/.agents/skills/engineering-principles/SKILL.md new file mode 100644 index 000000000..9d93595bc --- /dev/null +++ b/.agents/skills/engineering-principles/SKILL.md @@ -0,0 +1,272 @@ +--- +name: engineering-principles +description: >- + Language-agnostic engineering principles, and the trunk the language skills + branch from. Load it alongside python-code-factoring whenever you write, + refactor, restructure, fix or review code. Covers how to approach a change + in an existing codebase, how much weight an existing pattern deserves, + writing less code, the standards a test has to meet, and what belongs in a + comment. +--- + +# Engineering Principles + +These principles hold in any language, and cover how to approach the work rather +than its syntax. + +## How to Approach a Change + +Read the code the change touches, and the code around it. Then work in this +order: + +1. Work out what the change actually is. +2. Design the ideal implementation. Not the smallest diff: the code you would + write greenfield, knowing what you now know. What's already there is an input + to this step, not the answer to it. +3. Price the gap between that and what exists. Is it a breaking change or a + large refactor? Does it risk regressions somewhere you can't easily test? + Weigh that against a strong desire to leave the codebase simpler, more + modular and easier to extend than you found it. +4. If the full move really is too risky, do the largest part of it that still + leaves the codebase better than you found it. Not the smallest change that + works. + +Write like a senior engineer with the standing to improve the codebase, and hold +yourself to a higher bar than an average one trying to land their ticket. + +Expect the user to ask whether the fix really is as expensive as you claimed, so +check before saying it is. "We can do that in a follow-up" is usually laziness +rather than sequencing: if your change is what introduces the problem, fix it +here rather than filing an issue about it. + +The exception is work that's speculative rather than deferred. Park a refactor +grown out of proportion to a problem nobody has hit yet, and say so. + +### Consistency has a price, and it varies + +Existing patterns get their weight in step 3. Deviating from one fragments +something other people have to keep reading, so it costs something. How much +depends on where the pattern came from. + +- **Designed and reviewed by people, over time.** Breaking consistency with it + is expensive. Match it, and if you think it's wrong, say so rather than + quietly deviating. +- **Written by LLMs with light human steering.** The pattern may be one + unreviewed choice nobody vetted, so matching it propagates it. Weigh it + against the greenfield design on merit. + +When you can't tell, find out when the pattern arrived and who put it there, +then ask rather than guessing: + +> "If you're unsure whether to follow pattern or something potentially more +> correct, ask me." + +Introducing a new pattern has its own cost, so weigh "more correct" against +having two ways to do one thing. + +Being in the codebase is not the same as being current. A codebase often still +contains a pattern it is migrating away from, so check that what you're matching +is what it is moving toward. + +## Write Less Code + +The most common defect in agent-written code is code that didn't need to exist. +It arrives as helpers, wrappers, layers and guards, each individually +defensible, and it accumulates into something nobody can read. + +**Inline by default.** Something broken out and then called once is +indirection rather than abstraction, and a long parameter list on a private +method is the tell. Extract when the logic is complex enough to need a name, or +when a test needs an injection point. Check for prior art first: a helper that +duplicates one two modules over is worse than the repetition it removes. + +**Prefer repetition to indirection.** Aim for the dull middle: code that is +neither duplicated everywhere nor abstracted into something clever. Given a +choice between repeating four lines and introducing a type, repeat the lines. + +**Don't guard states that can't happen.** Trust a field that a schema, type or +admission check has already made required. Nil checks, `or` fallbacks, +truthiness guards and broad exception handling for impossible inputs add noise, +and when they do eventually fire they silently swallow real data. Push the +invariant out to the boundary that can enforce it, then rely on it. + +**Delete what a change makes redundant.** When you fix a root cause, remove the +workaround in the same change. When you replace a mechanism, remove what it +replaced. When a field, parameter or branch loses its last consumer, remove it. +Production code whose only callers are tests is dead code. + +**Don't paper over.** A suppression, an ignore comment, a hardcoded status, a +weakened schema, or an assertion adjusted to match the output are all ways of +deleting a signal rather than fixing what it reports. If a check is fighting +you, work out whether it's right before suppressing it. Suppress with a stated +reason only when the check is wrong about your code. + +**Fix the class, not the instance.** Once you know about a defect, look for the +rest of it before calling it fixed. The same bug usually sits in the sibling +code path, and the same smell elsewhere in your own diff. This holds whether you +found it or someone pointed it out. + +## Design for Change + +This applies to contracts, not to code structure. A schema, an API, a wire +format, or the signature of a function other people call are all expensive to +change once something depends on them, so choose shapes that evolve additively. +A field that might plausibly need more than one value wants to be a list, and a +boolean that might become three options wants to be an enum. Where you're +mirroring an upstream type, stay a strict subset of it so you can grow into it +later. + +It does not apply to code you're inventing. A function that might one day have a +second caller is speculative, and so is an interface with only one +implementation. Add them when something needs them. + +Ask what stage the project is at before paying to *avoid* a breaking change. +Where nothing is running the software yet, migration shims and deprecation +cycles buy nothing, so do whatever is most correct. + +That is not licence to design the contract carelessly. Aim for a contract that +never needs a new version, whether or not you could get away with breaking it +today. Being free to change something is a poor substitute for not needing to. +Where a restriction is easy to lift later, releasing it and waiting for someone +to complain still beats guessing. + +## Don't Defer the Hard Things + +In new work, the hard parts usually hold the risk you most want to retire early: +whether an idea is feasible, and whether the interface and the approach hold up. +Deferring them hides that risk until discovery costs most. + +Simplify the approach freely, with a naive algorithm, an in-memory store or a +hardcoded value. Don't simplify away the problem itself. Removing work is fine, +removing the question the work has to answer is not. + +Order the work by risk rather than by ease. Start with feasibility, then the API +design and data model, and then the choices that are expensive to reverse. +Integration points come next, and features come last. Everything above features +is a contract something else will depend on. + +These phrases are the tell that someone is deferring something hard because it's +hard: "for now we can just", "we can add it later", "to keep v0.1 simple", "we +don't need to worry about that yet". Sometimes the deferred thing really is +peripheral, so treat them as a prompt to check rather than a verdict. But if it +affects API shape, data model, or architecture, later means rewriting. + +None of this applies to a bug fix or an incremental change in a settled +codebase. There the risk is already retired, and the job is to fit in. + +## Verify, Don't Assume + +Confidence is not evidence. Before using an unfamiliar API, read its actual +documentation rather than recalling it from memory, because training data is +full of outdated signatures and plausible-but-wrong patterns. Before declaring a +change correct, run the tests, run the build, and read the diff with fresh eyes. + +## Comment the Why, Not the What + +The code already says what it does, so a comment that restates it is noise. +Comment what the code *can't* say, such as the reason for a non-obvious +decision. Above all, comment the external constraint behind it, such as an +upstream library's behaviour, a protocol quirk, an API that rejects the obvious +approach, or a bug you're working around. That context is expensive to +rediscover. + +```go +// SetConditions sets conditions to nil if passed no arguments. SSA interprets +// this as null and rejects it, so we only set them when there's something to set. +if len(cs) > 0 { + xr.SetConditions(cs...) +} +``` + +If a reader could derive the comment from the line below it, delete the comment. +If they'd need another package's source, a changelog, or an issue to understand +why the code looks this way, write it down. + +### Describe the State, Not the Change + +When you change code from approach X to approach Y, don't leave a comment saying +"do Y, not X". The reader has Y in front of them, and wants to understand why +the code is the way it is *now*. `Use a map here, not a slice` or +`Switched to SSA instead of patching` documents your edit, which belongs in the +commit message rather than the source. + +```go +// Bad — narrates the change. +// Use errors.Is here instead of == so wrapped errors still match. + +// Good — describes the current state and its reason. +// The client wraps this error, so compare with errors.Is rather than ==. +``` + +Mention the old approach only where a reader would reasonably wonder "why not +the obvious way?", or might want to refactor back to something that was bad for +a non-obvious reason. There it's part of the current code's rationale rather +than a history lesson. + +Mark deferred work with `TODO(user):`, and a decision that looks wrong until you +know the reason with `NOTE(user):`, so the next reader knows who to ask and that +it was deliberate. Phrase a TODO as the open question it is ("Use server-side +apply instead?"), not a vague "fix this later". + +## Structure for Testability + +Testability is a design property, decided when you structure the code. Code +that's hard to test is usually telling you the design is wrong: the code +hardwires dependencies it should accept as parameters, and mixes side effects +with logic in a function that does too much. + +Build in the injection points as you write. Being unable to test something +without standing up real infrastructure means you should restructure the code +rather than skip the test. + +### Test standards + +The language skills cover the local mechanics. + +- **Assert the whole output.** Compare the entire returned value against an + expected one. Picking out a few fields passes silently over the fields you + didn't think to check, which is where the bug is. +- **Cases are data.** Table-driven, with each case written out in full. + Repetition between cases is fine and factoring case construction into a helper + is not, because the helper hides what each case asserts. Deriving one case + from another by copying and mutating it hides it twice over. +- **A standalone test that runs the same path with different data is a case.** +- **Never compute the expected value with the code under test.** An expectation + produced by the production function passes under any implementation. +- **Fixtures for generated or third-party shapes are real artifacts.** Copy a + real one rather than hand-writing your approximation, and cover every variant + the code has to handle. A hand-written fixture tests your mental model. +- **Prove it can fail.** Break the behaviour, or revert the change, and watch it + go red. This matters most for concurrency, and for anything where a green run + might mean the fixture never built the condition or the test never executed + the path. +- **A test that only asserts the output hasn't changed is a change detector.** + It breaks on every legitimate edit and protects nothing. Assert a property + someone depends on, or don't write it. + +Follow the pattern already in the file. Where the change will merge with another +branch, follow that branch's pattern rather than the one on the main branch. + +## Errors Belong to Whoever Broke the Contract + +Work out whose invariant a failure violates before deciding how to handle it. A +malformed request from a caller is that caller's bug, and quietly compensating +for it hides the bug in both places. Report it rather than handling it silently. + +## Key Principles + +1. Design the ideal implementation first, then price the gap to what exists +2. Leave the codebase better than you found it, not minimally disturbed +3. Weigh an existing pattern by where it came from. Ask when you can't tell +4. Write less code. Inline by default, repeat rather than abstract +5. Don't guard states the schema already prevents +6. Delete what your change makes redundant, in the same change +7. Don't paper over a signal. Fix what it reports +8. Fix the class of defect, not the instance someone showed you +9. Design for change in contracts. Not in code you're inventing +10. Don't defer the hard things. Simplify the approach, never the problem +11. Verify, don't assume +12. Comment the why, not the what +13. Structure for testability. Seams are a design decision +14. Assert whole outputs against real fixtures, and prove the test can fail +15. Handle an error where its contract broke, not where it surfaced diff --git a/.agents/skills/krm-api-design/SKILL.md b/.agents/skills/krm-api-design/SKILL.md new file mode 100644 index 000000000..bda4dcbba --- /dev/null +++ b/.agents/skills/krm-api-design/SKILL.md @@ -0,0 +1,284 @@ +--- +name: krm-api-design +description: Design and review Modelplane's Kubernetes Resource Model (KRM) APIs - the XRDs under apis/. Use when designing, reviewing, or changing an XRD schema, or when asked how to structure fields in a Kubernetes API. +--- + +# KRM API Design + +Read `engineering-principles` first for how to approach a change at all. This +skill covers the schema. + +Modelplane's APIs are Crossplane XRDs under `apis/`, written as YAML OpenAPI v3 +schemas. An XRD schema is a thin wrapper on a CRD schema. The [Kubernetes API +conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md) +apply to both. Cite them rather than stating a preference. + +The goal is a great user experience for API consumers. Think about what it's like +to write YAML against this API and read it back six months later, and about +changing it without breaking anyone. + +## Design for Evolution, Not Versioning + +Above all, **design APIs that never need a new version.** + +API designers see version strings like `v1alpha1` and wrongly assume the path +to evolution is introducing `v1alpha2` or `v1beta1`. CRD versions +are two views into the same stored data. All versions must be round-trippable +to the storage version and back without data loss. This means: + +- You can rename or move a field across versions. +- You **cannot** add a new required field that doesn't exist in older versions. +- You **cannot** drop a field that an older version required. + +Introducing a new version gets you surprisingly little. The better path is +designing an API that can evolve with purely additive, backward-compatible +changes. Kubernetes evolved Deployment v1 for 7+ years without needing v2. + +## Terminology + +A thing of `kind: Foo` is a **custom resource**. A **CRD** is the *definition* +that allows it to exist. They're different objects and conflating them in code, +docs or review comments confuses readers who are still learning the model. + +## Breaking Changes + +Adding an optional field is not breaking. Changing a field's type, its casing, +or making it required is. Modelplane is pre-1.0 and nobody needs migrating, so +a breaking change that makes the API more correct is fine. It doesn't need an +alias, a deprecation or a migration path. + +Still, design each field so it never needs breaking, using the rules below. +Freedom to break the API today doesn't make a break free later. + +## The Core Rules + +### Use Required Fields Sparingly + +Assume you can never remove a required field. Ask: does the user really +need to express an opinion about this, or can the system pick a sensible default? + +If a sensible default exists, make the field optional with an explicit default +value. Explicit defaults are important, because they make the full desired state +visible rather than hiding it behind "unspecified means default behavior". + +### Prefer Enums Over Booleans + +This is the most common API design mistake. A boolean seems simple today +but almost always evolves into a set of options. + +```yaml +# Bad: what happens when you need a third option? +spec: + autoScaling: true + +# Good: room to grow. +spec: + scalingPolicy: Automatic # Could later add Manual, Scheduled, etc. +``` + +Many ideas start as boolean but trend toward a small set of mutually exclusive +options. Use a PascalCase string enum from the start. The cost is trivial (a +string field instead of a bool). The benefit is avoiding a breaking change when +the third option inevitably arrives. + +When you see yourself reaching for a bool, stop and think: is there a world +where this becomes three options? If there's even a chance, use an enum. + +### If in Doubt, Use an Array + +Could a field plausibly need multiple values in the future? Start with an array. + +```yaml +# Risky: what if you need multiple regions later? +spec: + region: us-east-1 + +# Safer: single-element arrays are fine. +spec: + regions: + - us-east-1 +``` + +Worst case, 99% of users use a single-element array forever. If you do need +more, you've saved yourself and your users from a painful migration. + +### Leave Room for Variants + +This is the most valuable pattern for future-proofing an API, and the one +Modelplane's maintainers raise most. Whenever you add something specific to one +cloud, engine, backend or vendor, ask whether there will be alternatives. There +usually will be. Put the specific fields under a **variant subobject** named for +the variant, and add a required **discriminator field** that says which +subobject applies: + +```yaml +apiVersion: modelplane.ai/v1alpha1 +kind: InferenceCluster +metadata: + name: us-west +spec: + cluster: + source: GKE + gke: + project: my-project + region: us-west1 +``` + +Adding EKS is then purely additive, as `source: EKS` with an `eks` object. +Don't infer the variant from which subobject the user happened to set, and don't +add cloud-specific fields to a shared object. An AWS-only field in a generic +`pools` block is the defect this pattern prevents. + +This pattern appears throughout well-designed Kubernetes APIs: + +| Discriminator | Variants | API | +|---|---|---| +| `mode` | `Pipeline` | Composition | +| `strategy` | `None`, `Webhook` | CRD conversion | +| `source` | `None`, `Secret` | FunctionCredentials | +| `provider` | `Cosign` | ImageConfig verification | + +**Choose a good name for the discriminator.** `mode`, `policy`, `strategy`, +`engine`, `backend`, `system` all tell you something about the domain. `type` is +acceptable when nothing more descriptive fits, but reach for a domain-specific +name first. + +**The discriminator should be a required enum.** Make the variant subobjects +optional, and add CEL validation rules that require the matching one, as +`apis/inferenceclusters/definition.yaml` does: + +```yaml +x-kubernetes-validations: +- rule: "self.source != 'GKE' || has(self.gke)" + message: spec.cluster.gke is required when spec.cluster.source is GKE. +- rule: "self.source != 'EKS' || has(self.eks)" + message: spec.cluster.eks is required when spec.cluster.source is EKS. +properties: + source: + type: string + enum: [GKE, EKS] + gke: + type: object + eks: + type: object +``` + +Worst case, you never need a second variant and some fields are slightly more +nested than necessary. If you do, you've saved yourself a breaking change. + +## Conditions + +Conditions are the standard way to communicate status. Get them right. + +- **Set conditions on first reconcile, even if status is Unknown.** This lets + other components know the controller exists and is making progress. +- **Provide a summary condition.** Use `Ready` for long-running resources, + `Succeeded` for bounded-execution resources. Simple consumers query just this. +- **Name conditions as adjectives or past-tense verbs.** `Ready`, `Available`, + `Succeeded`, `Failed`, rather than present-tense verbs like `Deploying`. +- **Choose polarity that's clearest for humans.** Which polarity is better + depends on the condition. `MemoryExhausted` may be clearer than + `SufficientMemory`. Avoid double negatives like `Failed=False`. +- **Reason is a one-word CamelCase identifier.** `BackendNotFound`, not + `"the backend could not be found"`. Message is the human-readable sentence. +- **Conditions are a map list.** Use `x-kubernetes-list-type: map` with + `x-kubernetes-list-map-keys: [type]`. + +## Printer Columns + +Define `additionalPrinterColumns` so `kubectl get` is immediately useful. + +- Surface the most important condition statuses (e.g. `READY`, `SYNCED`). +- Show key spec fields that identify or distinguish instances (e.g. `PACKAGE`, + `ENGINE`). +- Always include `AGE` as the last column. +- Use the jsonPath `$.status.conditions[?(@.type=='Ready')].status` pattern. + +## Field Design + +### Naming + +- JSON field names are camelCase. Kind names are PascalCase singular. Resource + plurals are all lowercase. +- Be declarative, not imperative. Name fields for what something **is**, not + what it **does**. +- Include units when ambiguous: `timeoutSeconds`, `retryLimit`, + `intervalMinutes`. Use `fooSeconds` for durations. +- Don't abbreviate unless extremely common (`id`, `args`). Acronyms should be + uniform case: `httpGet` at field start, `TCP` as a constant. +- Reference fields: `fooRef` for object references, `fooName` for by-name + references. Use `fooSelector` for label-selector-based references. + +### Bounds and Validation + +Every field needs bounds. This isn't optional. Unbounded fields are a DoS +vector and signal an unfinished API. + +- All strings: `maxLength` (and `minLength` if appropriate). +- All numbers: `minimum` and `maximum`. +- All arrays: `maxItems` (and `minItems` if appropriate). +- All maps: `maxProperties`. +- All enums: explicit `enum` list with PascalCase values. +- Use `format` where applicable: `int32`, `int64`, `date-time`, `byte`. +- Use CEL `x-kubernetes-validations` for cross-field constraints, immutability + (`self == oldSelf`), and conditional requirements. + +### Lists + +- Use lists of named subobjects, not maps of subobjects. The only exceptions + are pure key-value maps (labels, annotations, data). +- Declare merge semantics on every list: + - `x-kubernetes-list-type: map` with `x-kubernetes-list-map-keys` for + merge-by-key (e.g. conditions by `type`, ports by `containerPort`). + - `x-kubernetes-list-type: atomic` for replace-wholesale (e.g. enum lists, + short names). + +### References + +- Namespaced resources should reference objects in the same namespace. Cross- + namespace references breach security boundaries. If required, document the + edge cases and add permission checks. +- For references that could be by-name or by-selector, provide both + `resourceRef` and `resourceSelector` fields. Document that the ref takes + precedence when the user sets both. + +## Spec and Status + +- **Spec holds desired state. Status holds observed state.** They have separate + update paths via the `/status` subresource. +- **No extra top-level fields** beyond `spec`, `status`, and standard metadata. + If a resource doesn't need status (its state can't vary from intent), you can + omit status or rename spec to something domain-appropriate (e.g. `data`). +- **Spec fields are declarative.** They describe what the user wants, not what + actions to take. +- **Don't put "READ-ONLY" in a status field description.** Users can't write + to status fields. The note only raises the question of why this one said it. + +## CRD Metadata + +- Put every resource in a **category** so `kubectl get ` works across + all your resources. Choose a descriptive group-level category. +- Provide **short names** for resources users interact with frequently. Keep + them lowercase, short, and intuitive. +- Write a **resource-level description** that explains what the resource is in + one sentence, optionally followed by a link to documentation. +- Write clear **field-level descriptions** that explain semantics, constraints, + and interactions with other fields, rather than just restating the field's + name. + +## Key Principles + +1. Design for evolution, not versioning. You get less from a new version than + you think +2. Prefer enums over booleans. The third option always arrives +3. Use the discriminator + subobject pattern to leave room for variants +4. Choose descriptive discriminator names: `mode`, `strategy`, `engine` over `type` +5. Start with arrays when multiple values are plausible +6. Use required fields sparingly, and assume they're permanent +7. Put bounds on everything: strings, numbers, arrays, maps +8. Declare list merge semantics explicitly on every list +9. Set conditions on first reconcile, and provide a summary condition +10. Make `kubectl get` useful with printer columns that show conditions and key + fields +11. A thing of `kind: Foo` is a custom resource. The CRD is its definition +12. Being free to break an API is no substitute for not needing to diff --git a/.agents/skills/nix-flake/SKILL.md b/.agents/skills/nix-flake/SKILL.md new file mode 100644 index 000000000..053e3705b --- /dev/null +++ b/.agents/skills/nix-flake/SKILL.md @@ -0,0 +1,109 @@ +--- +name: nix-flake +description: Develop projects that use a Nix flake. Use when building, testing, linting, running dev commands, or entering a dev shell for a project that has a flake.nix. NOT for running ad-hoc CLI tools - use nix-tools for that. +compatibility: Requires Nix with flakes enabled and a project with flake.nix +--- + +# Developing with Nix Flakes + +## When to Use This (vs nix-tools) + +Use **this skill** when the project has a `flake.nix` and you need to: +- Build, test, lint, or run project-specific dev commands +- Enter or reference the project's dev shell +- Run flake checks (CI-style validation) + +Use **nix-tools** instead when you need to: +- Run an ad-hoc CLI tool (jq, ripgrep, etc.) that isn't project-specific +- Handle a "command not found" error for a general-purpose tool + +## Discovery + +Before running anything, read `flake.nix` to understand what's available. Flakes +expose a standard set of output types. Check which ones the project defines: + +| Output | Purpose | Command | +|--------|---------|---------| +| `packages` | Build artifacts | `nix build` or `nix build .#` | +| `checks` | CI checks (tests, lints) | `nix flake check` | +| `apps` | Runnable dev commands | `nix run .#` | +| `devShells` | Development environment | `nix develop` | + +To list what's available without reading Nix code: + +```bash +nix flake show # Tree of all outputs +``` + +## Common Workflows + +### Building + +```bash +nix build # Build the default package +nix build .# # Build a specific package +``` + +`nix build` symlinks build results to `./result`. + +### Running Checks + +```bash +nix flake check # Run ALL checks (tests, lints, etc.) +``` + +You can't run individual checks in isolation via `nix flake check`. If the +project exposes the same functionality as apps (e.g., `nix run .#test`), prefer +that for faster feedback during development. Reserve `nix flake check` for full +CI-style validation. + +### Running Dev Commands + +```bash +nix run .# # Run a specific app +nix run .# -- # Pass arguments to the app +``` + +Apps are project-specific commands (test, lint, generate, etc.) defined in the +flake. They run outside the Nix sandbox with full filesystem and network access. + +### Dev Shell + +```bash +nix develop # Enter the default dev shell +``` + +The dev shell provides all project tools (compilers, linters, CLI tools) in a +reproducible environment. It often prints available commands on entry. + +You don't typically need to enter the dev shell yourself. The `nix run` and +`nix build` commands already use the flake's dependencies. The dev shell is for +interactive use by the developer. + +## Passing Arguments + +Use `--` to separate Nix flags from program arguments: + +```bash +nix run .#test -- -v -run TestFoo +``` + +## Debugging Failures + +If a command fails: + +1. Check `nix flake show` to verify the output exists +2. Read the relevant section of `flake.nix` to understand what the command does +3. For build failures, `nix log` shows the build log of the last failed derivation +4. For apps, the source is usually a shell script in a `nix/` directory - read it + to understand what it runs under the hood + +## Key Concepts + +- **Flake outputs are hermetic**: builds and checks run in a sandbox with no + network access and only declared dependencies. Don't expect them to behave + like running commands directly. +- **Apps are not sandboxed**: unlike builds and checks, apps have full system + access. They're wrappers around normal development commands. +- **`nix flake check` runs everything**: it evaluates all checks for the current + system. There's no built-in way to run one check in isolation. diff --git a/.agents/skills/nix-tools/SKILL.md b/.agents/skills/nix-tools/SKILL.md new file mode 100644 index 000000000..1680192fb --- /dev/null +++ b/.agents/skills/nix-tools/SKILL.md @@ -0,0 +1,72 @@ +--- +name: nix-tools +description: Run any CLI tool via Nix - assume tools like jq, yq, ripgrep, and fd are available. Use CLI tools freely for JSON/YAML processing and searching. Also use when a command is not found. +compatibility: Requires Nix with flakes enabled +--- + +# Running Tools with Nix + +This system has Nix available. You can run virtually any CLI tool without it being permanently installed. + +## When to Use This + +Assume any CLI tool you know about is available. Use the right tool: + +- **JSON/YAML**: `jq` for JSON, `yq` for YAML - great for Kubernetes manifests +- **Searching**: `ripgrep` (rg) for content, `fd` for filenames + +Also use when: +- A command fails with "command not found" +- You need a specific or newer version of a tool + +## Running Tools + +### One-off command + +```bash +nix run nixpkgs# -- +``` + +Examples: +```bash +nix run nixpkgs#jq -- '.foo' file.json +nix run nixpkgs#yq -- '.metadata.name' manifest.yaml +nix run nixpkgs#ripgrep -- 'pattern' . +nix run nixpkgs#fd -- '\.go$' +``` + +The `--` separates nix options from the program's arguments. + +## Common Package Names + +Most packages use the tool's common name: + +| Tool | Package | +|------|---------| +| jq | `nixpkgs#jq` | +| yq | `nixpkgs#yq` | +| ripgrep (rg) | `nixpkgs#ripgrep` | +| fd | `nixpkgs#fd` | +| bat | `nixpkgs#bat` | +| tree | `nixpkgs#tree` | + +## Finding Packages + +If unsure of the exact package name, search nixpkgs: + +```bash +nix search nixpkgs +``` + +Note: search is slow (evaluates all packages). Prefer trying the obvious name +first - it usually works. + +## Newer Tool Versions + +If the default nixpkgs has an outdated version, use nixpkgs-unstable: + +```bash +nix run github:NixOS/nixpkgs/nixpkgs-unstable# -- +``` + +This tracks the latest available versions in nixpkgs. diff --git a/.agents/skills/python-code-factoring/SKILL.md b/.agents/skills/python-code-factoring/SKILL.md new file mode 100644 index 000000000..27b846c04 --- /dev/null +++ b/.agents/skills/python-code-factoring/SKILL.md @@ -0,0 +1,215 @@ +--- +name: python-code-factoring +description: Factor and structure Python code for clarity, testability, and maintainability. Use when writing, refactoring or reviewing the structure of Python code, or adding type annotations. Also use when deciding where constants or modules belong, or when the user mentions factoring, refactoring, or code organization in Python. Also use before writing Python unit tests. +--- + +# Python Code Factoring + +Read `engineering-principles` first. It covers how to approach a change, and the +language-agnostic rules for writing less code and for comments and tests. This +skill covers what is specific to Python. + +For Crossplane composition functions, `crossplane-python-functions` covers the +compose loop, required resources, readiness and gating. + +[references/example.md](references/example.md) has real before and after pairs +for the rules below, taken from the reviews that produced them. + +## Write for the Next Reader, Not the Compiler + +> "It should be friendly and easy to understand to newbies, even if that means +> doing something that might be a little less DRY or efficient." +> +> "if reading these functions just ends up like reading controller-runtime code, +> but in Python, we've failed." + +Put legibility ahead of both DRY and efficiency. The hardest thing in a file +should be the domain rather than the machinery. + +Prefer one pattern that scales up and down over two right-sized ones. + +## Use the Typed Model + +Where a generated or declared model exists for a value, use it. Raw dicts are +the fallback for data that has no model, not a convenience for when the model is +awkward. + +```python +# Good. +fs = filesystem.FileSystem.model_validate(observed) +name = fs.metadata.annotations["name"] + +# Bad. Untyped, and every key is a typo waiting to happen. +name = to_dict(observed).get("metadata", {}).get("annotations", {}).get("name") +``` + +The places this gets missed: + +- **Reads**, not just writes. Validate into the model, then use attribute + access. A chain of `.get()` calls is the same defect as building a dict. +- **Tests**, for the XR. Composed and observed resources are the exception: + tests write them as wire-form dicts, as the Tests section of CONTRIBUTING.md + explains. +- **Later changes.** Reintroducing a dict is a regression, including in a file + that already contains one. + +When a serialization quirk makes the model awkward, change the schema that +generates it, or drop the field if nothing reads it. Reaching around the model +at the call site is the wrong repair. + +## Never Hand-Edit Generated Code + +Generated models, clients and schemas are outputs. If one is wrong, fix the +generator or its input and regenerate with the project's own build target. That +includes merge conflicts: resolve a conflict confined to generated files by +regenerating, never by hand. + +## Boring Python + +Prefer the plain construction a reader can follow without decoding it. + +- Free functions and simple classes over inheritance hierarchies. Where several + modules share a structure, let convention carry it and skip the base class. +- No metaclasses, descriptor tricks or intersection types. A type-level + construct a reader has to decode is a cost even when it's correct, and many + boring suppression comments beat one clever type. A `Protocol` is fine when it + buys something concrete, such as decoupling a helper so it can move upstream. +- No serialization gymnastics where plain assignment works. Round-tripping a + value through `model_dump` and `model_validate` to reach an identical schema + is overcomplicating it. +- Comprehension golf needs a comment. Nobody can read + `max((-(-len(x) // w) for w, x in live), default=1)` without one. + +Underscore prefixes on the methods of one entry-point class read as noise to +people who don't work in Python daily. Use them for a real internal boundary +rather than by reflex. + +## Names and Locals + +**Don't alias an expression that already reads well.** `xr.spec.name` beats +`name = xr.spec.name`, which is three characters. Several call sites don't +rescue a local either. Keep one when it names something the expression doesn't +say, or when it caches an expensive call. + +**Guard clauses over nesting.** Return early on each failure rather than +building a compound condition. If a reader has to work out what +`if a and not b and c not in d` means, restructure it into ordered returns. + +**Don't accumulate state on `self` to pass it between methods.** Attributes are +for what belongs to the object for as long as it exists, such as the request and +response. A cluster of `self.auth_*` fields that have to remain consistent is +either a value type or, more often, a set of arguments. + +**Too many arguments is a smell.** Trace the concrete inputs and outputs of each +step to find the right place to split, and drop any parameter the function can +derive from another one it already has. + +## Modules and Constants + +Choose a module's name for what it holds, so the module name does half the +naming work at the call site: `status.populate(xr)` reads better than a +`populate_status` that lives wherever was convenient. A name that says nothing +about its contents, `consts` being the usual offender, attracts unrelated code. + +A module's name is a contract. `metadata.py` holds Kubernetes object metadata, +so an unrelated constant doesn't go there just because it needs a home. + +Extract a module when it has a consumer today. + +For constants: + +- A string that steers control flow doesn't belong inline. Reuse the generated + enum value where the schema has one, otherwise make it a constant. +- Make a value that forms a contract between components a constant, but leave + an ordinary config argument as a literal. +- Declare the constants a module needs at the top of that module, where a reader + can see its whole vocabulary at once. +- Prefer per-module constants over a shared constants package, and accept the + duplication. The Crossplane CLI tooling fights a shared package, and symlink + hacks cost more than the duplication does. +- Never alias an imported constant into a module-local name. + +## Imports + +Google Python style. Import modules, not the names inside them. + +```python +# Good. +from myproject import conditions +c = conditions.Condition(...) + +# Bad. +from myproject.conditions import Condition +c = Condition(...) +``` + +Match the codebase's existing aliases rather than inventing new ones, and use +the conventional ones where they exist, such as `metav1` for `meta.v1`. + +## Type Annotations + +Annotate function declarations, and have the linter enforce it rather than +relying on review. + +Only add annotations a checker actually verifies, and wire that checker into the +project's own tooling. An unchecked annotation is a claim nothing tests. + +When the checker complains, first ask whether it's right. + +- In library or production code a type error usually reports a design problem. + A value that is never `None` shouldn't have an `Optional` type. A helper only + tests call belongs in the tests. Fix the design rather than the annotation. +- A targeted suppression with a stated reason is acceptable, and is better than + a clever type. Prefer replacing it with a small function that raises a + descriptive error where that's available. A blanket ignore, an + `assert x is not None` added to quiet a checker, or a cast asserting what you + wish were true are all papering over. +- In table-driven tests, suppress a rule that fights the canonical table + layout, such as a statement-count limit on a long table, because the rule + doesn't understand the pattern. + +## Errors and Inputs + +`assert` is for programmer errors. A schema can permit something your code +doesn't handle, and turning that into an `AssertionError` traceback gives the +caller nothing to act on. Raise something descriptive instead. + +Don't mutate inputs. Return a populated copy if you need defaults applied, so a +later reader can still tell what the caller actually sent. + +## Docstrings + +Give each function and helper a docstring. The reader can already read the +code, so make it short. One that grew a paragraph per round of review wants +rewriting rather than extending, and fifty lines to explain eight environment +variables is a defect rather than thoroughness. + +## Tests + +`engineering-principles` covers the principles. In Python: + +- **Follow the Tests section of CONTRIBUTING.md.** It's the reference: plain + pytest functions over a table of `Case`s, with no classes or fixtures. +- **Compare at the highest level the test allows**, in one assertion over the + whole response rather than several over slices of it. +- **Build the XR from its model, and write resources as dicts.** The generated + models fill in schema defaults the function doesn't emit, so a composed or + observed resource built from one won't match the real response. +- **Start without helpers** and add one only when it cuts real noise. Helpers + that build an individual resource are fine. Helpers that build whole cases are + not, and neither is deriving one case from another by copying and mutating it. + +## Key Principles + +1. Legibility beats DRY and efficiency. One pattern that scales up and down +2. Use the typed model everywhere one exists, reads and tests included +3. Never hand-edit generated code. Fix the generator and regenerate +4. Boring Python. No cleverness a reader has to decode +5. Don't alias expressions that already read well +6. Guard clauses over compound conditions and nesting +7. Name modules for what they hold +8. Import modules, not names +9. Annotate declarations, and only with what a checker verifies +10. A type error reports a design problem before it reports a missing ignore +11. `assert` is for programmer errors, not for input the schema permits +12. Docstrings are short. Tests are tables of models diff --git a/.agents/skills/python-code-factoring/references/example.md b/.agents/skills/python-code-factoring/references/example.md new file mode 100644 index 000000000..742d45889 --- /dev/null +++ b/.agents/skills/python-code-factoring/references/example.md @@ -0,0 +1,125 @@ +# Worked Examples + +Real before and after pairs from composition function code, taken from the +sessions where these rules got made. The "before" in each case is what an agent +wrote and Nic corrected, so treat the left column as the thing to avoid. + +Identifiers are as they appeared, minus anything environment-specific. + +## Reading through a typed model + +The rule applies to reads, not just writes. A chain of `.get()` calls has the +same defect as building a dict: nothing checks the keys. + +```python +# Before. +def _observed_efs_filesystem_id(self): + observed = self.req.observed.resources.get("efs-filesystem") + if not observed: + return None + manifest = resource.struct_to_dict(observed.resource) + return manifest.get("metadata", {}).get("annotations", {}).get("crossplane.io/external-name") + +# After. +def _observed_efs_filesystem_id(self): + observed = self.req.observed.resources.get("efs-filesystem") + if not observed: + return None + fs = fsv1beta1.FileSystem.model_validate(resource.struct_to_dict(observed.resource)) + if not fs.metadata or not fs.metadata.annotations: + return None + return fs.metadata.annotations.get(_ANNOTATION_EXTERNAL_NAME) +``` + +The read now goes through the generated model, and the magic string became a +constant. The `if not observed` guard stayed, because a resource +really can be absent on an early reconcile. That's the distinction: guard a +state that happens, drop the guards for states the schema rules out. + +## Dropping guards the schema already makes impossible + +Same file, later in the same session. Every `x if x and x.y else` chain here was +defending against a field the XRD marks required. + +```python +# Before. +env_name = env.metadata.name if env.metadata else "" +capacity = env.status.capacity if env.status and env.status.capacity else None +backend = capacity.backend if capacity else "" + +if engine not in _COMPAT.get(backend, []): + continue + +gpu_pools = capacity.gpuPools if capacity and capacity.gpuPools else [] + +# After. +env_name = env.metadata.name if env.metadata else "" +backend = env.status.capacity.backend or "" + +if engine not in _COMPAT.get(backend, []): + continue + +gpu_pools = env.status.capacity.gpuPools +``` + +`capacity` existed only to hold the defensive chain. Once the guards go, so +does the variable. + +## Locals that only shorten an expression + +The same loop, one pass later. + +```python +# Before. +for p in all_placements: + p_labels = p.metadata.labels if p.metadata and p.metadata.labels else {} + p_deployment = p_labels.get("modelplane.ai/deployment", "") + if p_deployment == xr_name: + continue + p_ie = p.spec.inferenceEnvironmentRef.name if p.spec and p.spec.inferenceEnvironmentRef else "" + if p_ie == env_name: + p_gpu_count = 0 + if p.status and p.status.resources and p.status.resources.gpu: + p_gpu_count = p.status.resources.gpu.count or 0 + used_gpus += p_gpu_count + +# After. +for p in all_placements: + p_labels = p.metadata.labels or {} + p_deployment = p_labels.get("modelplane.ai/deployment", "") + if p_deployment == xr_name: + continue + p_ie = p.spec.inferenceEnvironmentRef.name if p.spec and p.spec.inferenceEnvironmentRef else "" + if p_ie == env_name: + used_gpus += p.status.resources.gpu.count or 0 +``` + +`p_gpu_count` names nothing the expression doesn't already say, so four lines +become one. `p_labels` and `p_deployment` survive, because they hold a magic +string and a comparison that reads better named. + +## Inlining a value used once + +Final pass over the same block. + +```python +# Before. +backend = env.status.capacity.backend or "" + +if engine not in _COMPAT.get(backend, []): + continue + +gpu_pools = env.status.capacity.gpuPools +best_gpus_needed = None +for pool in gpu_pools: + +# After. +if engine not in _COMPAT.get(env.status.capacity.backend or "", []): + continue + +best_gpus_needed = None +for pool in env.status.capacity.gpuPools: +``` + +Each of the three passes removed a name rather than adding structure, and the +code got shorter every time. diff --git a/.agents/skills/writing-style/SKILL.md b/.agents/skills/writing-style/SKILL.md new file mode 100644 index 000000000..a0a919c7e --- /dev/null +++ b/.agents/skills/writing-style/SKILL.md @@ -0,0 +1,438 @@ +--- +name: writing-style +description: Write and review prose in Modelplane's style. Use when drafting or editing designs, docs, code comments, field descriptions, commit messages, PR descriptions or review comments. Also use when reviewing the prose in someone else's change, or when tightening, de-slopping, or copy editing any prose. +--- + +# Writing Style + +Modelplane's prose follows the style of Nic Cope, its lead maintainer. The +maintainers read every word and delete whatever the reader doesn't need, so aim +for a draft they'd leave as it is. The quotes below are Nic's corrections to +agents, kept as evidence for each rule. + +Most of this skill covers what goes wrong. An LLM's instinct for a good design +doc is roughly right about voice and section structure, and badly wrong about +length, honesty about rationale, and who the document addresses. So the failure +modes come first. + +For voice, the section-by-section skeleton, and worked examples of the target, +read [references/voice.md](references/voice.md). Read it when drafting from +scratch. Skip it when tightening prose that already exists. + +## When to Use This (vs Other Skills) + +Use **this skill** for how prose reads, wherever it appears: designs, docs, code +comments, XRD field descriptions, commit messages, PR descriptions and review +comments. CONTRIBUTING.md sets the conventions specific to commits, PRs and +issues, and "Cut Hard, Twice", "Never Invent Rationale", and "Substantiate or +Delete" below apply to them unchanged. + +Use **design-docs** for the structure of a design doc. + +Use **reviewing-pull-requests** for what to raise in a review, and this skill +for how to word it. + +## Reviewing Someone Else's Prose + +The failure modes below are also the checklist for reviewing prose in a PR. +Raise the defects a reader would trip on, such as a claim the code doesn't +support or invented rationale. A comment three times longer than it needs to be +is another. Quote the text, say what's wrong, and leave the rewrite to the +author. + +CI already runs Vale over `docs/`, so don't repeat what it reports there. +Elsewhere, such as design docs and code comments, raise a pattern of AI-written +phrasing in one comment. + +## Before You Draft + +Establish three things, and ask rather than guess: + +1. **Audience.** Who reads this? Maintainers, contributors, users, a partner's + engineers? "High level but deeply technical" produces a different document + from "internal, to align on asks". +2. **Purpose.** What decision does it serve, or what does the reader do + differently afterwards? +3. **Premise and scope.** What does this document assume, and what falls outside + it? State the premise in the document rather than hedging across several + scenarios. + +Almost every framing correction traces back to a document drafted without a +specific reader in mind. Naming the reader up front prevents most of them. + +**Public documents argue technical merit alone.** Where the real motivation is +commercial, find the honest technical reason or don't write it. Never dress a +commercial motive as engineering rationale, and never write a circular one. +"We're adding support for this engine because we said we'd add it" is not a +background section. If you can't find a real reason, say so and ask. + +## The Failure Modes + +### Cut Hard, Twice + +Your first draft runs about 40% longer than it should. That's predictable enough +to treat as a step in the process. Draft, cut 30%, then read it again and remove +the 10-20% still there. Nic's calibration, verbatim: + + +> Even when I ask you to cut 30% of what you write I can easily find another +> 10-20% that can be cut with zero loss. + + +Do both passes before anyone reads it. A draft the reader has to cut costs them +the time the document should have saved, and Nic has said plainly that +repairing LLM prose is expensive: + +> Generally when you write a document it costs me a lot of time spent getting it +> to an actually good state. + +Delete first: + +- Sentences restating the previous sentence in different words. +- Throat-clearing and hedging preambles. +- Anything a competent author would obviously have done. +- Explanation the reader can reconstruct from what you already wrote, or from a + link. Link out instead of paraphrasing. +- Trailing summaries, recaps, and closing questions. An assessment is a + statement. +- Background the argument doesn't rest on. Explain only the property the + argument needs: that Volcano and Kueue can gang schedule LeaderWorkerSet, + without explaining how either works. + +Accuracy is never a reason to grow. When correcting a claim, keep the fix inside +the existing footprint. "Give me a more accurate version of my last paragraph +that isn't any longer" is a routine request, as is a copy edit capped at one or +two extra lines. + +Prose is the expensive part of a document. Trading paragraphs for a worked YAML +example is usually a win. After a concrete sketch, delete the prose that repeats +it and keep only what the sketch can't express. + +Depth of thinking and length of output vary independently. "Think hard, but +answer succinctly" is a coherent instruction. + +### Write for the Reader, Not Against the Conversation + +Nic makes this correction more sharply than any other, and you won't infer it, +because from inside the session the reactive version feels responsive. + + +> You're once again doing the thing where: A) you write a doc, B) I criticize a +> point, C) you edit the doc to specifically react to that point, explicitly +> naming the point (without reason) throughout. For example your doc now +> litigates what requirements are due to fleet and what aren't, when the reader +> doesn't actually care. + +> This is garbage writing. You're writing this section in reaction to the +> discussion we just had. Think about the audience. Think about the flow of the +> document. Rewrite the section from first principles. + + +It appears two ways. The document argues with a reviewer's objection, or it +argues with its own earlier draft. Phrasings like "this replaces an earlier +approach", "GPUs per node isn't a…", and "not a fleet layer on top of a cluster +level platform" all give it away. + +To catch it, check whether a sentence would make sense to somebody who never +read the review thread or the previous draft. Delete it if it wouldn't. The +reader has access to neither. + +Feedback changes what a section says, and never makes the section defend itself. +So rewrite from first principles, positively, as though nobody had raised the +objection. Patching won't do. A framing error in one sentence usually means the +section needs redoing, and patching prose in place is how documents become +incoherent. + +Where a reader might reasonably ask "why not do it the other way", answer that +in Alternatives Considered. An inline rebuttal is the wrong home for it. + +### Never Invent Rationale + +The reason behind a decision comes from the author's intent. A diff shows a +rename without saying why. A plausible reason invented to fill the gap is worse +than no reason, because the reader can't tell it apart from a real one and it +becomes part of the permanent record. + +If you don't know why, ask. Nic has said so outright: + +> DO NOT invent justification unless you know for sure why something is like it +> is. You can ask me if you don't know. + +When he catches it the response is blunt: "You just made that all up." + +This extends past rationale: + +- **Invented terminology.** Don't coin a phrase that sounds like a term of art + and isn't. Check a term has currency in the domain before asserting it does. +- **Invented APIs.** Never show a field, resource, or flag that doesn't exist, + above all in another project's API. +- **Invented policy.** Don't document a rule that nothing enforces. +- **Invented examples.** See "Examples Are Real" below. + +An Alternatives Considered entry records what actually happened, and each +argument attaches to the alternative it truly applies to. + +### Substantiate or Delete + +Assertion is not argument. "Dynamo's KV cache offloading is ahead of vLLM" needs +the mechanism and the baseline: ahead how, and what does vLLM do? Expect the +question "why?" about any sentence claiming a causal necessity, and "what's your +source for that insight?" about any confident claim. A sentence you can't defend +is filler, so remove it. + +Root claims in primary evidence. For behaviour the code decides, over the docs +and over the README. + +Word choice must not imply a conclusion the facts don't support. Describing a +cache as LRU to suggest it's worse, when it also supports frecency, is a writing +defect as much as a factual one. Evaluative adjectives need a definition and +even application. Don't call one project's feature "experimental and niche" +without assessing its peers the same way. + +Avoid a partisan frame. A comparison of two stacks is not a contest between us +and them, and rhetorical absolutes rarely survive scrutiny. + +### Space Signals Importance + +The room a topic gets is how a reader judges its importance, so proportion +is a property to check. Nic checks it by eye and by line count: "Your new +section takes up more space than any other in the doc. Should it?" + +- Sections stay proportional to their importance to the whole document, never to + how recently they came up. Recency bias is a defect. A commit message follows + the diff, not the order the work happened in. +- List entries stay proportional to their siblings. Any number of entries is + fine. One entry three times the size of the others because it's your current + focus is not. +- After trimming one section, trim its neighbours. Cutting one part leaves an + adjacent part oversized. +- Real content doesn't belong in parentheses. Anything that merits a mention + merits a sentence, and something that deserves billing equal to its peers + deserves a paragraph. +- One concept per list entry. Don't bundle rate limiting and authentication + under "identity", and don't drop an item while reorganising. + +### Write the Design as If It Preceded the Code + +A design records direction. It concentrates on APIs and UX with broad-strokes +mechanics. It doesn't mirror the code, and it isn't a recipe to build it. It +must not actively mislead, though it may show things not yet built. + +So write in the present tense, describing the current position as though it were +the first and only one: + +- No changelog framing, since the reader doesn't care how a previous + unreleased iteration worked. +- Discarded thinking belongs in Alternatives Considered, gathered there rather + than scattered inline. +- No links to issues or PRs in the same repo. They expose the document's real + chronology, and a design should read as the first artifact in the repo. Link + generously outward to upstream issues, specs, talks, and prior art. +- A superseded document isn't rewritten. It gets a one or two sentence preface + saying it remains for historical context. +- Don't link a durable document to a scratch file you'll delete. + +At some point a design stops absorbing revisions and gets extended by +supplementary designs. Prefer minimal surgery to a rewrite, because it's cheaper +to review. + +A proposal proposes the intended end state, and leaves out the limitations +currently blocking it. Goals appear positively, as what gets delivered, and at +project level rather than scoped to a release. Don't confess open unknowns in +passing. Stay deliberately vague where the mechanism remains unsettled, and +state the requirement instead. + +Leave out an Open Questions section. A design takes a position. Opinions belong +inline beside the thing they concern, rather than in a trailing "positions" or +"recommendations" section. + +### Examples Are Real + +An example does more work than the prose around it, so give it the same +scrutiny. + +- **Reuse validated artifacts from the repo.** Copy the tested manifest instead + of composing one from memory, and use the same example throughout the + document, including the executive summary. +- **Show whole objects.** A full ModelDeployment, and not a `spec:` fragment. +- **If you can't verify it, say so.** "I couldn't build a credible example here" + is better than a plausible fabrication. +- **Calibrate realistically.** Selectors should ask for what a model actually + needs, and examples should reflect the scale users really run. +- **Cut examples that don't do any work.** One that only shows an off-the-shelf + resource doesn't add anything a reader lacks. Merge a pair that make the same + point. A contrived example damages credibility more than an absent one, so + omit rather than force. +- **Annotate lightly.** Brief YAML comments where semantics aren't obvious, and + `# Omitted for brevity` where you trim. +- **Cover the awkward case.** Worked examples are how a proposal proves it + handles the topologies it claims to, so include the pathological one. + +Long verified artifacts belong in collapsed blocks, so they illustrate without +bloating the document. + +## Naming + +Naming draws more corrections than anything but length, so treat it as work that +precedes the prose. + +- Offer two or three candidates with trade-offs instead of one answer. +- Prefer terms already established in the domain. Where people contest a + category label, say which specific things you mean instead. +- Don't borrow a loaded term unless you've adopted its semantics. Using DRA's + "attributes" promises DRA's behaviour. +- Test a candidate by saying the sentence a reader would say aloud. "Configure + serving" beats "configure the endpoint". +- Reject names that mislead in the degenerate case. "Leader" is wrong when + there's one pod, and "gang" is wrong for a gang of one. +- Weigh precision against length. A more specific name is also longer, which + costs something. +- Consider what a name connotes. "Size" suggests how big, where "count" + suggests how many. +- One term per concept, across prose, field names, and environment variables. A + document drifting between "stacks" and "backends" has a bug. +- No blanket bans. The repair for one inaccurate use of "orchestrator" is + accuracy in that instance, rather than global find and replace. +- Re-derive the title and filename at the end, from what the document argues. + Titles are pithy and descriptive. Anything that sounds like a slogan is out. + +## Formatting + +Reasoning belongs in prose. Reserve structure for content with real +structure. + +- **No decorative structure.** Bold, headings, and bullets sprinkled in to look + organised are a tell. A pair of paragraphs doesn't need an `###`. Resist + subheadings in PR descriptions altogether. + +- **Consistent heading hierarchy**, and headings that sound like an engineer + wrote them. "The shape I'm proposing" is not one. + +- **Ordered lists for sequential content**, prose for reasoning, bullets for + parallel items. Parallel content gets parallel structure and matched depth. + Labels in a parallel list run one to three words and stay consistent with each + other. +- **Tables** where the content really is tabular. Give a table a lead-in + sentence saying what it shows, use title case headers, and make column names + convey their role. Avoid a hyphen as a placeholder, because it reads as + "none". +- **Real links.** A bare `#34` doesn't resolve in a repo markdown file. Link + code through a pinned-commit permalink so GitHub renders it as a snippet. Keep + named projects hyperlinked. +- **No mathematical shorthand** in prose. Spell out what a Σ means. +- **Wrapping** depends on where the text appears. Hard-wrap commit messages and + repo markdown at 80. Never hard-wrap GitHub issue or PR bodies, which reflow + to the viewport. +- **Introduce a term before relying on it.** "The composition function" is a + defect if the document hasn't said it's built on composition functions. Avoid + forward references to concepts the reader hasn't met. +- **Don't make the reader infer.** If CUDA matters to two things, introduce it + for both. + +Grammar gets its own pass. Run-on sentences are the recurring offender. + +## The Pass Before You Hand It Back + +Nobody should spend reading time on an un-de-slopped draft. Nic, to an agent: + + +> I want you to tackle the prose cuts and AI tells edits before I spend time +> reading. This should be a tight design that reads like I wrote it. No slop, no +> filler. + + +Getting there takes two passes, which catch different things. + +**Vale catches the mechanical fingerprints.** The project lints prose with the +[`vale-ai-tells`](https://github.com/tbhb/vale-ai-tells) rules. Its 110 rules +cover em dashes, filler, hedging, puffery, figurative verbs, rhetorical +scaffolding, and the rest of the phrase-level fingerprints. Some rules from the +Google developer documentation style run alongside them: `Passive`, +`Semicolons`, `ExcessiveClaims`, and `Anthropomorphism`. The rest of that style +stays off, since it bans first person and "we", which this skill relies on. + +`.opencode/vale/.vale.ini` holds that config. Sync its styles once, then lint: + +```bash +vale sync --config .opencode/vale/.vale.ini +vale --config .opencode/vale/.vale.ini path/to/doc.md +``` + +The docs under `docs/` have their own, stricter config, which CI runs. + +**Fix what it reports.** Nic compared 16 linter-driven rewrites against the +originals across two design docs. He preferred the rewrite in 14 and called the +other two a tie. He never preferred the original. His summary: the edits drive +toward more succinct and direct phrasing, and that's the project style. + +So comply by default, and override only when you can state the specific reason +the alert is wrong. + +Be suspicious of your own sense of which alerts are noise, because it runs in +one direction: you will defend phrasing you wrote. Every alert in that +comparison that an agent had confidently called a false positive turned out to +be a real catch. The most confident defence was `StackedAnaphora` on "one +engine, one member, one pod", argued as deliberate repetition mapping onto the +API. Nic's verdict was that it "reads very AI", and the repetition was the +reason. + +The exceptions are mechanical rather than matters of taste: + + +- `UniversalObject` where the quantifier is the semantics. In a matching rule, + "satisfies every selector" is precise, and cutting "every" makes it wrong. +- `ColonUsage` firing on a document's metadata block or on YAML, where the + parser reads a field name as prose. + + +For prose a person wrote, assume they meant it, and raise the alert rather than +acting on it. + +Where a document must quote a banned construction to discuss it, wrap that +region in `` and `` rather than leaving an +alert. + +Where your harness reports Vale's alerts as diagnostics on each edit, fix those +as part of the edit. A harness may cap how many it reports per edit, so a clean +edit doesn't mean a clean file. Run `vale` directly before handing the document +over. + +**A subagent catches what Vale can't see.** Vale matches phrases. It can't tell +whether a section argues with a reviewer or invents a reason, or whether it +gives four paragraphs to something that deserves one. Dispatch a subagent +briefed with [references/de-slop-review.md](references/de-slop-review.md), plus +the audience, the purpose, and one or two existing designs under `design/` for +the project's style. Fix what it finds and repeat until a reviewer comes back +clean, because one pass rarely suffices. + +Use a subagent rather than reviewing inline. Fresh context matters because the +drafting agent can't see its own reactive framing. + +Where accuracy matters, run a separate pass that tests every assertion against a +skeptical reader rather than a friendly one. Prose review and correctness review +are orthogonal. + +## Do Not + +- Don't add AI attribution. Text published under a person's name reads as + theirs, so write first person or drop the pronoun, and never refer to the + author in the third person. +- Don't lose a person's edits. Where someone copy edits alongside you, their + edits win, and any later rewrite keeps them. Re-read the file before + editing. +- Don't summarise material the reader wrote or already knows. + +## Key Principles + +1. Cut 30%, then cut another 10-20%, before anyone reads it. +2. Write for the reader, never against the last draft or the last comment. +3. Never invent rationale. Ask instead. +4. Support every claim with a mechanism and a baseline, or delete it. +5. Space signals importance, so keep sections and list entries proportional. +6. A design reads as though it preceded the code. +7. Examples are real, whole, verified, and doing work. +8. Name things deliberately, with established terms that don't mislead in the + edge case, and one term per concept. +9. Reasoning belongs in prose. Structure is for structured content. +10. Run Vale, then a fresh-context subagent, and loop until both come back + clean. diff --git a/.agents/skills/writing-style/references/de-slop-review.md b/.agents/skills/writing-style/references/de-slop-review.md new file mode 100644 index 000000000..03063188a --- /dev/null +++ b/.agents/skills/writing-style/references/de-slop-review.md @@ -0,0 +1,141 @@ +# De-slop Review + +A briefing for a fresh-context reviewer. Dispatch a subagent with this file, the +document under review, and the answers to the three questions below. Read it +yourself only to check what you're asking the reviewer to do. + +Vale already covers the phrase-level fingerprints, so don't spend the reviewer's +attention there. Run Vale as SKILL.md describes first and fix what it reports. +This review covers the defects Vale can't detect, because they depend on what +the document means rather than which words it uses. + +## Briefing the Reviewer + +Tell the reviewer: + +1. **Audience.** Who reads this document. +2. **Purpose.** What decision it serves. +3. **Project style.** Point at one or two existing designs under `design/`, so + the reviewer can compare rather than guess. + +Then give it the instructions below. + +## Instructions for the Reviewer + +You're reviewing a technical document drafted by an LLM for Modelplane's +maintainers. They read every word and delete whatever the reader doesn't need. +Your job is to find what they would object to, before they spend time on it. + +Read the whole document first. Then work through the checks below. Quote the +offending text for each finding, because a claim the author can't locate is a +claim they'll ignore. + +### 1. Reactive framing + +The worst and most common defect. The document argues with something the reader +can't see: a reviewer's objection, or the document's own earlier draft. + +Look for sentences that only make sense to somebody who followed the drafting +conversation. Symptoms include a passage litigating a distinction the reader +never raised, comparisons against an earlier approach the reader can't see +("this replaces an earlier…", "X isn't a…", "not a Y on top of a Z"), and a +section that reads as a rebuttal rather than an explanation. + +Report each one, and say what the section would look like written from first +principles for the stated audience. + +### 2. Invented specifics + +Flag anything the author couldn't know or the reader can't verify: + +- A reason for a decision that reads as reconstructed rather than remembered. +- A field, resource, flag, or API that may not exist, above all in a third + party's project. +- A term presented as established usage. +- A rule or policy that nothing enforces. +- An example that looks composed from memory rather than copied from something + tested. + +Check these against the repo and upstream sources where you can. Where you +can't, say which claims you couldn't confirm rather than passing them. + +### 3. Unsupported claims + +For every comparative or evaluative statement, ask whether the document supplies +the mechanism and the baseline. "X is ahead of Y" needs to say how, and what Y +does. Flag: + +- Confident claims with no traceable source. +- Evaluative adjectives applied to one thing and withheld from its peers. +- Word choices implying a conclusion the facts don't support. +- Rhetorical absolutes. +- A partisan frame in what should be a neutral comparison. + +### 4. Proportion + +Space is how a reader judges what matters, so measure it. Report any section or +list entry noticeably larger than its siblings, and say whether its importance +justifies that. Watch for a topic inflated because it came up late in drafting. +Check that real content isn't hidden in parentheses, and that no list entry +bundles several concepts. + +### 5. Flow and coherence + +Read the document end to end as somebody meeting it for the first time: + +- Does an opening paragraph make a promise that the section never delivers on? +- Does the document use a term before introducing it? +- Do two passages say the same thing in different places? +- Is there a paragraph left over from an earlier framing that no longer applies? +- Does the ordering respect what the reader knows so far? +- Are there run-on sentences? + +### 6. Structure + +Check the document against the design-docs skill's skeleton: executive summary, +background, goals, proposal, alternatives considered, with future improvements +where they apply. Report: + +- An Open Questions or Risks section, when a design should take a position. +- A trailing positions, recommendations, or summary section. Opinions belong + beside the thing they concern. +- Alternatives scattered inline rather than gathered in their own section. +- Headings, bold, or bullets used for decoration. Reasoning belongs in prose. +- Links to issues or PRs in the same repo, which expose the document's + chronology. +- Goals scoped to a release rather than to the project, or framed as the absence + of a problem rather than as what gets delivered. + +### 7. Length + +Say where 20% could come out with no loss of meaning, and quote the candidates. +Assume the draft is still too long even after the author has cut it once. If you +find nothing, say so plainly rather than inventing a suggestion. + +## Report Format + +``` +## Verdict +Ship, or needs work. One sentence. + +## Findings +For each, in priority order: +- **[check number and name]** — quoted text. What's wrong, and what to do about it. + +## Could not verify +Claims you were unable to confirm, with what you tried. + +## Length +Where the next 20% comes from, quoted. +``` + +## Reviewer Constraints + +- Don't edit the document. Report, and leave the file alone. +- Don't rewrite passages wholesale unless asked. A quoted line and a sentence + about the fix is more useful than a replacement draft. +- Don't restate the phrase-level matches Vale already caught. +- Don't open with praise or close with encouragement. The author reads the + findings. +- Don't soften a finding to seem agreeable. A defect you mention gently goes + unfixed. diff --git a/.agents/skills/writing-style/references/voice.md b/.agents/skills/writing-style/references/voice.md new file mode 100644 index 000000000..d222bc6ec --- /dev/null +++ b/.agents/skills/writing-style/references/voice.md @@ -0,0 +1,254 @@ +# Voice and Structure + +What the target looks like, drawn from ~30 of Nic Cope's documents spanning +2019-2025. Read this when drafting from scratch. The failure modes in SKILL.md +matter more, because they describe where drafts actually go wrong. This file +describes where they should end up. + +## Voice + +### First person, direct + +Write in first person. "I propose…", "I believe…", "I think…", "I'd argue…". +State positions plainly instead of hedging behind passive voice or committee +language. The reader should always know what the author thinks and why. + +Use "I" rather than a collective "we" for a document one person authored. Where +uncertain, say so ("I'm not sure how real or widespread these concerns are", "I +suspect…") while still taking a position. + +### Conversational authority + +The tone is an experienced engineer talking to peers. Informal enough for +contractions and for phrases like "a lot of", "pretty complex", "not so bad". +Rigorous enough for precise technical detail, concrete numbers, and YAML. + + +Avoid corporate voice ("leverage synergies", "drive alignment"), academic voice +("it is posited that"), and stacked hedges ("it might perhaps be worth +considering"). + + +### Empathy for the reader + +Assume a smart reader who lacks your context. Give enough background for +somebody joining the conversation, without over-explaining what peers know. +Where a concept surprised you, say so, then explain what made it click. + +Take care with `obvious`, `obviously`, `clearly`, and `of course`. What's +obvious to a senior engineer often isn't obvious to a junior one, and these +words risk making a reader feel bad for not already knowing. This isn't a hard +ban, since some things really are obvious. Default to stating the thing rather +than editorialising about how apparent it should be. + +Use analogies to connect unfamiliar ideas to familiar ones. "An MRD is to an MR +CRD as an XRD is to an XR CRD." "Think of this like AWS Lambda." "`flake.nix` is +a bit like a Makefile backed by a snapshot of nixpkgs." + +### Release verbs point forward + + +Nic uses "ship" for the act of releasing, as intent or as something still ahead. +"We need to ship X." "Should v0.1 ship with something simpler?" "Biasing toward +shipping rather than another review round." + +He never uses it to describe what a product has or does. "Modelplane ships a +Traefik gateway" and "nobody ships this layer today" are LLM phrasings. The fix +is the specific verb: Modelplane *installs* a Traefik gateway, *includes* the +connector runtime, *composes* the HTTPRoute. Naming the real action is more +precise anyway, which is what the linter asks for when it flags the word. + + +Frequency isn't the problem, so don't overcorrect into avoiding the word. Nic +uses it about as often as an LLM does. The difference is the direction it +points. + +The same test applies to any verb of delivery or provision with a product as its +subject. Say what the software does, and reserve the release verb for releases. + +## Structure + +### Standard sections + +Most documents follow this skeleton, though few use all of it: + +1. **Title.** Short, descriptive, re-derived at the end from what the document + argues. +2. **Status, date and author.** As the existing designs under `design/` do. +3. **Executive summary** (longer docs). A few paragraphs giving the whole + picture, so a busy reader can stop there. Include the primary example here. +4. **Background.** What the reader needs to follow the proposal, including + prior art and the problem. +5. **Goals.** What the document tries to achieve, and what falls outside it. +6. **Proposal.** The concrete design. +7. **Workstreams** or **Migration plan** (optional). What makes it real. +8. **Future improvements** (optional). Out of scope now, plausible later. This + is where speculative material belongs. +9. **Alternatives considered.** What else you evaluated, and why it lost. + +Shorter one-pagers skip the executive summary and run background, proposal, +alternatives. Drop any section with nothing to say. + +There is no Open Questions section. See SKILL.md. + +### Front-load the destination + +Before the background starts building the case, give the reader a sentence or +two saying where this goes. It can close a brief introduction or open the +executive summary. Knowing the thesis turns the background from "where is this +going?" into "I see why this matters". + +### Background is a story + +Background sections run long relative to their neighbours, because they tell +the story of how we got here and what changed. They link out to +prior work so the reader can go deeper. A good one has the reader nodding along, +thinking "yes, I see why this is a problem", before the proposal starts. + +Long is not the same as padded. Each paragraph still has to do work, and the +section still gets both cutting passes. Where the background outgrows the rest +of the document by a wide margin, that's a finding. + +Background also needs the case that forced the decision. Where prefill/decode +disaggregation was the litmus test for a new design, the background covers it. +Showing the status quo in YAML often beats describing it. + +### Goals are crisp + +Goals sections are short, a few bullets or a brief paragraph. They state what's +in scope and what isn't. "It's not a goal to…" appears often. + +Pitch goals at project level rather than at a release, and state them +positively, as what gets delivered. Keep build detail out. Goals also frame +ambition: "My goal with this document is to sketch out…" sets different +expectations from "The goal of this proposal is to have Upbound Crossplane +install only the CRDs customers actually use." + +### Proposals are concrete + +Proposals include YAML, protobuf, API specs, and diagrams. Show the API rather +than describing it. Walk through the user experience step by step: + + +> Assume you've installed provider-aws-ec2 with mrdActivationPolicy: Manual… +> You then install a Configuration that depends on provider-aws-ec2. The +> Configuration would consider its dependency satisfied, because the Provider is +> installed. The Configuration wouldn't be usable though… + + +Where a proposal claims to handle a set of cases, prove it with a worked example +per case, including the pathological one. + +### Code is for developer experience + +Design documents aren't the place to sketch how the code works inside. Leave out +Go or Python unless the code is itself the design, as with a library API, an +SDK, or a developer experience where what the user writes is the thing you're +designing. + +Show code where the argument concerns how something would feel to use. Leave it +out where it would only illustrate the innards of a controller or a reconciler. +Those belong in the repo. + +### Alternatives are fair + +Give each alternative a fair hearing. Say what it is, what's good about it, then +why the proposal is better, without resorting to strawmen. This applies doubly +where the alternative is the current system. + +Include an alternative only when it adds something. Not every discarded idea +needs an entry, and a low-value one is better deleted. Keep entries roughly the +same length as each other. + +## Rhetorical patterns + +### Build the case before the proposal + +Structure the document so the reader reaches the proposal already understanding +the problem. By the time they hit "I propose…", it should feel like the natural +next step. + +### Acknowledge trade-offs + +Don't pretend the proposal is perfect. State the downsides: + +> PR code running in the VM can rewrite the agent's instructions mid-review, so +> a PR can steer its own review. It's still only a comment, so we accept that. + +### Use concrete numbers + +Prefer specific numbers to vague claims: + +> 90 superfluous CRDs × 1.75 MB = 157.5 MB of API server memory. That's not so +> bad for one control plane, but across 10 control planes it's 1.5GB of wasted +> memory. + +Numbers have to be real, and traceable to something you measured or read. The +number above comes from a measured CRD size. A plausible figure invented to make +a paragraph concrete is the worst kind of fabrication, because its precision is +what makes the reader trust it. Where you don't have a number, describe the +effect and say you haven't measured it. Never estimate hardware costs, +latencies, or utilisation rates to strengthen an argument. + +### Respect what came before + +Assume whoever designed the previous system did what they could with what +they knew, and assume they're reading this. Catalogue concrete shortcomings +("the ingestion pipeline adds substantial machinery for a debugging tool") +without calling the old system bad or broken. Focus on what changed, meaning new +requirements or constraints, that makes the old approach a poor fit now. Explain +why it's time to move on rather than belittling the past. + +### Address skeptics directly + +Anticipate objections and answer them inline. State the likely objection in the +reader's voice, take it seriously, then answer it. Don't introduce each one with +the same formula. + +Note the boundary with reactive framing in SKILL.md. Answering an objection a +reader would independently raise is good. Rewriting a section to argue with a +reviewer who already raised one is not. + +### Frame decisions as preferences + +"I'd prefer…", "I lean toward…", "I don't feel very strongly" signal confidence +in the reasoning without shutting down discussion. + +Where constraints forced a choice rather than taste, say that instead. Traefik +is the only thing meeting the requirements today. Framing it as a favourite +misleads. + +### Reference prior art + +Link to and summarise relevant prior work, such as upstream issues, old design +docs, community discussions, and conference talks. Show awareness of a problem's +history. + +Link outward, generously. Don't link to issues or PRs in the same repo, for the +reasons in SKILL.md. + +## Formatting + +### Paragraphs and flow + +Write flowing paragraphs rather than bullet-heavy outlines. Use bullets for +lists of concrete items such as API fields, workstreams, or goals, and prefer +prose for reasoning and narrative. Paragraphs run three to six sentences. + +### Headings + +Use descriptive headings. "Namespace Composite Resources" beats "Proposal Item +1". H2 for major sections, H3 for subsections, applied consistently. + +### Code and YAML examples + +Include examples inline where they clarify the proposal. Add brief comments +where semantics aren't obvious. Keep examples focused, trimming irrelevant +fields with `# Omitted for brevity`. Show whole objects rather than fragments, +and reuse tested manifests from the repo. + +### Links + +Use inline links in prose, or reference-style links at the bottom for upstream +design docs. Make issue references real URLs, since a bare `#34` doesn't resolve +in a repo markdown file. diff --git a/.opencode/vale/.gitignore b/.opencode/vale/.gitignore new file mode 100644 index 000000000..810fe7c6b --- /dev/null +++ b/.opencode/vale/.gitignore @@ -0,0 +1 @@ +styles/ diff --git a/.opencode/vale/.vale.ini b/.opencode/vale/.vale.ini new file mode 100644 index 000000000..2688017cf --- /dev/null +++ b/.opencode/vale/.vale.ini @@ -0,0 +1,19 @@ +# Lints the review agent's output as it writes it. The ai-tells rules catch +# the phrasing that marks text as agent-written. Four Google rules apply to +# review comments too. The rest of the Google style stays off, since it bans +# the first person that review comments use. +StylesPath = styles +MinAlertLevel = error +Packages = Google, https://github.com/tbhb/vale-ai-tells/releases/download/v1.37.0/ai-tells.zip + +[*.md] +BasedOnStyles = ai-tells + +# "Implementation" is the precise word in an engineering review, and the +# plainer alternatives this rule suggests are worse. +ai-tells.FormalRegister = NO + +Google.Anthropomorphism = error +Google.ExcessiveClaims = error +Google.Passive = error +Google.Semicolons = error diff --git a/nix/checks.nix b/nix/checks.nix index ad7e251d9..1ae843ecf 100644 --- a/nix/checks.nix +++ b/nix/checks.nix @@ -38,6 +38,29 @@ in # docs-site repo, which builds and link-checks it where it deploys. docs-vale = docs.vale; + # Lint the skills under .agents with the ai-tells rules and a few Google + # ones, using the Vale config in .opencode/vale. Reuses the docs check's + # synced style packages. + agents-vale = + pkgs.runCommand "modelplane-agents-vale" + { + nativeBuildInputs = [ + pkgs.vale + pkgs.findutils + ]; + } + '' + export HOME=$TMPDIR + cp ${self}/.opencode/vale/.vale.ini .vale.ini + mkdir styles + cp -r --no-preserve=mode ${docs.valeStyles}/* styles/ + find ${self}/.agents -name '*.md' -print0 | \ + xargs -0 --no-run-if-empty \ + vale --config="$PWD/.vale.ini" + mkdir -p $out + touch $out/.agents-vale-passed + ''; + # Validate the example manifests the docs show against the generated Pydantic # models, so an example that drifts from the live API schema fails CI. Covers # everything under docs/manifests/, including the API-reference examples under diff --git a/nix/docs.nix b/nix/docs.nix index 3b3f0bf55..94f1c6181 100644 --- a/nix/docs.nix +++ b/nix/docs.nix @@ -45,6 +45,9 @@ let ''; in { + # The synced style packages, for other checks that lint with Vale. + inherit valeStyles; + # Lint docs prose with Vale. Merges the network-synced style packages with the # repo's local Modelplane style and vocabulary into one StylesPath, then lints # offline. From 22e1190a81ab8d93d3fb6d647cc296880bc6b0a2 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Sat, 31 Oct 2026 23:06:43 -0700 Subject: [PATCH 02/13] Add skills for reviewing pull requests and designs Agent reviewers find problems indefinitely, and the author's agent addresses each one. Most of what maintainers decline falls into a few kinds: backward compatibility for pre-1.0 software, guards for states the schema rules out, and confident claims about external tools that turn out to be false. reviewing-pull-requests sets a bar a finding must pass before it's posted, what not to raise, and how to phrase what is: correctness and design feedback as a question, an explanation and a suggestion, style feedback directly. reviewing-designs holds the questions maintainers ask of a proposal, such as what the user gets and whether the next engine would need a new Modelplane release. Both are distilled from my review sessions and the comments maintainers posted on Modelplane PRs, and carry real findings from this repo for calibration. Signed-off-by: Nic Cope --- .agents/skills/reviewing-designs/SKILL.md | 176 +++++++++++ .../references/calibration.md | 128 ++++++++ .../skills/reviewing-pull-requests/SKILL.md | 288 ++++++++++++++++++ .../references/calibration.md | 115 +++++++ 4 files changed, 707 insertions(+) create mode 100644 .agents/skills/reviewing-designs/SKILL.md create mode 100644 .agents/skills/reviewing-designs/references/calibration.md create mode 100644 .agents/skills/reviewing-pull-requests/SKILL.md create mode 100644 .agents/skills/reviewing-pull-requests/references/calibration.md diff --git a/.agents/skills/reviewing-designs/SKILL.md b/.agents/skills/reviewing-designs/SKILL.md new file mode 100644 index 000000000..419e246f2 --- /dev/null +++ b/.agents/skills/reviewing-designs/SKILL.md @@ -0,0 +1,176 @@ +--- +name: reviewing-designs +description: >- + Judge whether a Modelplane design or approach is a good one, from whether + its problem is real to whether its API will hold up. Use when reviewing a + design doc, when checking a PR's premise against its issue and the committed + designs, or when weighing alternatives. For the quality of a design doc's + writing, use design-docs. +--- + +# Reviewing Designs + +This covers whether a proposal is good, not whether it's well written. It's +distilled from how Nic Cope questions designs, and from the design concerns +Modelplane's maintainers raised on design PRs and what came of them. + +Maintainers own design decisions. As a reviewer, ask the questions a +maintainer would ask and leave the answers to them, since you're better at +making sure a question gets asked than at deciding it. + +## What's Settled and What Isn't + +Treat a design doc committed on the base branch as settled. Review a PR against +it, and don't argue with it. Where a PR departs from it, ask whether the +departure is deliberate. Don't ask whether the committed design was right. + +A design doc the PR adds or changes is under review, so review it. A design +records direction, mostly API and UX. It doesn't have to match the code +exactly. A design that actively misleads about how the system works is a +finding. + +Reopening a settled decision is sometimes right, when the PR shows the decision +doesn't hold. Say so plainly: "this reopens X from the design, because Y". + +## The Questions + +These are the questions maintainers ask of a proposal, roughly in the order +they matter. + +### What does the user get? + +- What problem does this solve, and for whom? How does it help them compared + with today? +- Who consumes each new field, status or resource, and how? Follow every + artifact to whoever creates it and whoever reads it. +- Could a user do this today without the change, with the right image, flags + or existing resources? +- Does it matter in practice, or is it an optimization nobody would notice? + Ask for numbers where the case rests on them, such as how long a user waits. + + +> What is the purpose of ModelCache? With pre-staging, the 45 minutes is paid in +> advance, so the user experience for an MD author is "my model is ready in ~15 +> mins". + + +### Does each piece need to exist? + +- Could this layer, field, resource or code path go? What would a smaller + version lose? +- Is there an existing primitive that already does this, such as a + Composition, a second ModelDeployment, or `podSpecTemplate`? Raise the + reuse-what-exists alternative even if you expect it to lose. +- Does a new concept duplicate one Modelplane already has? +- Is an override or escape hatch covering for a default that the PR should fix + instead? + +> Could we drop this layer? Layer 2 already checks every class a cluster +> actually uses. + +### Will the API hold up? + +The krm-api-design skill has the rules. The questions that matter most: + +- Would a new engine, engine version, router, cloud or accelerator need a new + Modelplane release? Test the API against a second one, such as SGLang beside + vLLM, or Dynamo beside llm-d. +- Does it encode one engine's or one cloud's specifics into a generic API? Does + it leak an implementation detail, such as LWS, InferencePool or the EPP's + config, that users then depend on? +- Does it encode today's topologies into the schema, rather than letting users + describe their own? +- Where there might be alternatives, is there a required discriminator and a + subobject per variant? Adding a required field later breaks every user. +- Does it future-proof only where adding something later would break users? A + list of enum values reserved for imagined futures is speculative. +- Is upstream configuration that changes often, such as engine flags, left + opaque? Is a stable upstream API, such as a pod template, mirrored as a + compatible subset? + +> Would a new vendor need a Modelplane release to support this? It looks like +> the routing block encodes vLLM's P/D flags. + +### Does it fit? + +- Does it hold to the principles in the committed designs? Re-check each + revision against them, since a fix for one problem can break a principle, + such as "Modelplane doesn't inject engine flags". +- Does it treat like things alike? If users set one option directly while + Modelplane picks a sibling option for them, what's the reason? +- Who configures each part, the platform team or the model's author? Is the + split principled? +- Does it use upstream projects the way their authors intend, or invent its own + interpretation? How much is upstream and how much is ours? +- What does it ask of people who extend Modelplane, such as a composition + author for a new cloud? +- Does it overlap an open PR or issue? +- Does a user need to understand Crossplane to use it? Needing it installed is + fine. Needing to understand it isn't. + +### Does it work? + +- Has the author shown someone could build it? Are there gaps in how it would + work? +- What happens at the edges? A user omits a field, hardware autoscales, a + dependency fails permanently, or requests are in flight during a change. +- When it fails, which way does it fail? A default is fine if getting it wrong + costs a little quality. It isn't if it produces something that reports Ready + and rejects requests. +- Does it assume something of clients that real clients don't do, such as + sending a special header? +- Does it make trying Modelplane out harder, such as needing a real domain or a + TLS certificate before the first request? +- Does it judge by today's scale, or by the ceiling it needs to reach? Judge by + the ceiling. + +### Is anything hard deferred? + +Deferring is fine when the design could add the deferred part later without a +breaking change. It isn't fine when the deferred part determines the API's +fields or structure. Phrases like "for now we can just" and "we can add it +later" signal a deferral to check. Ask whether the API would still fit once +someone builds the deferred part properly. + +## Alternatives + +- Is each rejected alternative given its strongest form? What would make it + work? +- Is the reason it lost the real one? If the real reason is governance or + ecosystem, the doc should say so rather than give a technical one. +- Is there an obvious alternative missing, such as reusing what exists, or what + a comparable project does? + +## What Not to Raise + +- Breaking changes and migration. Modelplane is pre-1.0. A breaking change that + makes the API better is welcome. +- Settled decisions in committed designs, or ones a thread already settled. +- Code-level detail in a design doc. Designs cover concepts, APIs and UX. +- Requests to make a design match its implementation exactly. +- Omissions the doc or the PR marks as out of scope or deferred, where adding + them later wouldn't break anything. +- Commercial or strategic motives. Public designs argue technical merit. +- Detail that wouldn't change the design, such as a timeout value. + +## How to Raise It + +Phrase design questions as reviewing-pull-requests asks for any correctness +question: ask, explain, suggest. A design question especially wants a hedge, +because you're missing context the author has. + +- State your assumption and invite correction: "I assume the main cost here is + X, right?" +- Sketch an alternative in YAML where it helps, and say you haven't thought it + through: "please poke holes in this rather than taking it as-is". +- Give the cost of your suggestion, not only its benefit. +- Separate a gap in the design from a gap in how the doc explains it. +- Offer more than one option where they come naturally, and say when you don't + mind which. + +Getting a fact wrong costs the whole review its credibility. Check a claim +about how Crossplane, Kubernetes or an upstream project behaves against its +source before you ask a question that rests on it. + +[references/calibration.md](references/calibration.md) has real design concerns +from Modelplane's history and what came of them. diff --git a/.agents/skills/reviewing-designs/references/calibration.md b/.agents/skills/reviewing-designs/references/calibration.md new file mode 100644 index 000000000..08c245f6d --- /dev/null +++ b/.agents/skills/reviewing-designs/references/calibration.md @@ -0,0 +1,128 @@ +# Calibrating Design Questions + +These are real design concerns from Modelplane PRs, and what came of them. Use +them to judge which questions change a design, and how to ask them. + +## Changed the design + +On #362, the ModelCache design: + + +> What is the purpose of ModelCache? Is it: 1. Get weights on a cluster before +> any model replica runs there 2. Get weights on a cluster the first time a +> model replica runs there 3. A bit of both? ... It was something like 45 +> minutes to hydrate the cache PVC, then another 15+ minutes to load from the +> cache PVC. + + +The design was rebuilt around staging weights in advance. The reviewer asked +what the user would experience, with the options laid out and numbers on the +wait. + +Later on the same PR, a revision began injecting `--model` into the engine: + + +> Doesn't this go against our overarching design principle of not injecting +> engine args though? What if a new engine shipped tomorrow that used a +> different flag for this? + + +A fix for one problem broke a principle the committed design states. Check +each revision against those principles, not only the first draft. + +On #514, the prefill/decode routing design: + + +> This seems like a case where we'd need to ship a new Modelplane to support +> new engine features - e.g. new types of KV transfer. ... I don't think +> there's a principled reason for MD to be how it is today: with all engine +> configuration exposed to the user but (almost) all routing configuration +> abstracted and/or opinionated. + + +That question led to four revisions of the API. Testing a design against an +engine or router it doesn't name yet is the most productive question in +Modelplane's design history. + +On #112, an API with one source of models so far: + + +> I think we should add a required discriminator field now, even though we +> only support HF. ... I prefer required discriminator fields because it forces +> the user to be explicit about what they want, and makes the code that handles +> the API simpler. + + +The field went in. A required field can't arrive later without breaking users, +so add it before the second variant exists. + +On #363, the metrics design, a question that followed the data to its reader: + + +> I think we need to step back and think about what we want to help folks +> monitor. + + +The review asked who reads each kind of metric and how, and the design moved +its on/off switch to where the people who consume the metrics configure them. + +## Raised, and the proposal held + +On #116, a design for prefill/decode deployments: + + +> Another alternative that comes to mind: could you just write two MDs? One for +> prefill and the other for decode? It feels really close to what's proposed, +> and I like that it just reuses existing primitives. However I think what's +> proposed is better + + +The reviewer raised the reuse-what-exists alternative and argued it fairly, +then concluded the proposal was better. It went into the doc's alternatives. +Asking was right even though the answer was no. + +## Declined + +On #64, reserving enum values for APIs Modelplane might support one day: + + +> Closing — extension path can be designed when a real second API lands. + + +Future-proof where adding something later would break users. Don't enumerate +imagined futures. + +On #542, a request for a heartbeat in case the cluster agent froze: + + +> A frozen agent makes pools look fuller, not emptier. The scheduler keeps +> charging every new replica, because it can't count one as observed until the +> agent recovers. + + +The failure already erred in the safe direction. Ask which way a failure errs +before proposing a mechanism to prevent it. + +On #422, a request for per-component overrides of the serving stack's +defaults: + + +> Values like PDBs and security contexts: if our defaults are missing a strict +> non-root context or a PDB, that is something we should fix in our/aicrs +> defaults, not a missing override. Otherwise every team fixes the same thing + + +An escape hatch can paper over a default that should change. Ask whether the +default is what's wrong. + +## What to take from this + +- The questions that changed designs asked what the user gets, or whether the + API would need a new release for the next engine. Ask those first. +- Ask about an alternative even when you expect the proposal to hold, as long + as you argue the alternative fairly. +- A concern about a hypothetical future, or a failure that's already safe, + rarely changes anything. +- Some concerns need context no agent has, such as a decision made in Slack or + where a release's scope ends. Where a design seems to rest on something you + can't see, ask about it rather than assuming it's missing. diff --git a/.agents/skills/reviewing-pull-requests/SKILL.md b/.agents/skills/reviewing-pull-requests/SKILL.md new file mode 100644 index 000000000..5b80f60ee --- /dev/null +++ b/.agents/skills/reviewing-pull-requests/SKILL.md @@ -0,0 +1,288 @@ +--- +name: reviewing-pull-requests +description: >- + What to raise when reviewing a Modelplane pull request, and how to phrase + it. Use when reviewing someone else's PR, when + drafting review comments or a review summary, or when triaging findings from + a review agent or bot. +--- + +# Reviewing Pull Requests + +This covers what Modelplane's maintainers look for in a PR and how they phrase +it. It's distilled from about 4,500 of Nic Cope's review comments on +Crossplane and Modelplane, and from the findings he declined. + +CONTRIBUTING.md wins wherever it disagrees with this skill. Don't spend +comments on anything CI already enforces: ruff, ty, Vale on the docs site, the +function test-style checker, the list-selector checker and manifest validation. + +## The Bar for Posting + +A reviewer told to find problems will always find some. Agent reviewers find +them indefinitely, and with the author's agent addressing each one, a low bar +turns review into churn. Post a finding only if all of these hold: + +- **It's true, and you verified it.** You traced the code path or ran the + test that shows it. A claim about how Crossplane, Kubernetes, a + provider or any other tool behaves needs a link to the source or docs that + shows it. If you can't verify it, don't post it. +- **This PR introduced it.** A pre-existing problem is not this PR's finding, + unless the PR spreads it further. +- **It can happen.** A failure needs a concrete input or sequence that causes + it, in a deployment someone would run. A state the schema, the type or an + admission rule already rules out is not a finding. +- **The author would want to change it**, knowing everything you know. + Interesting is not the same as important. + +Don't raise these at all: + +- **Backward compatibility, migration or upgrade paths.** Modelplane is + pre-1.0 and nobody needs migrating. Challenge compat shims and migration docs + instead. +- **Guards for states that can't happen.** A runtime check for something the + XRD schema or a CEL rule enforces puts one invariant in two places. +- **A second enforcement point.** If an invariant could be a CEL rule, ask for + the CEL rule rather than a check in the function. +- **Alternative architectures or new features.** Design and API direction + belong to the maintainers. Where a change has real design weight, ask about + it once in the summary. Don't spread it across inline comments. +- **Settled questions.** Read the existing review threads first. Don't re-raise + something already discussed, resolved, or declined. +- **Missing E2E coverage.** E2E runs in a separate, label-gated workflow. +- **Performance micro-optimisations** without a measured problem. +- **Documentation of ordinary language semantics**, such as who owns a map a + caller passes in. + +[references/calibration.md](references/calibration.md) has real findings and +what happened to them. Read it before deciding what to post. + +## How to Phrase Feedback + +**Frame correctness and design feedback as a question.** That covers bugs, +architecture, API shape, and docs or comments that say something untrue. Use +three parts: ask, explain, suggest. + +1. Ask about the problem you think you see, not about your fix. +2. Explain why you think so, hedged as what it looks like from here: "It looks + like...", "I think...". +3. Suggest an alternative, also as a question, when one is clear: "What if + we...?", "Could we...?". Where two or three fixes come to mind, offer them + all, and say when you don't mind which. Don't invent options to have more + than one. A bare question is fine when no fix is clear. + +You may be missing context, and a question invites the author to supply it. +Authors also feed review comments to their own agents, which take a comment +stated as fact at its word and act on it even when your analysis is wrong. +Asking makes them check. Ask even when you're confident. + +Offering options matters for the same reason. An agent handed one fix applies +it as written. Handed a few, or told the reviewer doesn't mind how the problem +gets solved, it has to weigh them, and may find a better fix than any of yours. + +> Does an absent `draDrivers` need to mean "install everything"? It looks like +> compose-inference-cluster always sets the list, so this path only serves +> ServingStacks created before the field existed, and Modelplane has none to +> migrate. What if we required the field, or defaulted it to `[]`? That would +> remove the `None` branch in `join`, the `ExistingStandardUnfiltered` case and +> the comments in both functions that explain it. + +> Won't returning here delete a running cluster? I think Crossplane garbage +> collects composed resources that drop out of the desired state. Could the +> gate keep composing what already exists? + +> Is the list below still right for AMD pools? It says an existing cluster +> provides its own exporter and needs NVIDIA's dcgm-exporter chart, but this +> paragraph says the serving stack installs the AMD exporter. Should it say +> that only applies to NVIDIA pools? + +Phrase a correctness finding in the review summary this way too. + +**Be direct about style, readability and maintainability.** "Please use the +established pattern here, because..." is fine, as long as the comment gives the +reason. Where it helps, link to CONTRIBUTING, a skill or an existing example in +the repo. The author should come away knowing the convention, not only with a +fixed line. Mark minor points with `Nit:`. + +> Nit: "Gets deployments" sounds like this will return deployments, when it +> actually seems to apply the supplied ModifyFn. + +> This is documenting a change. Document how it works, not how it differs +> from how it used to work. (Here and elsewhere.) + +Write short comments. Most run two to four sentences, ending with the +suggestion. +Make one point per comment, attached to the line it's about, or for code the +PR didn't change, to the change that causes the problem. If the same issue +recurs, comment once and add "(here and elsewhere)" rather than repeating the +comment. + +A comment that proposes an alternative may also sketch it, as a short YAML or +code block with a sentence or two on what it buys. Show only the parts +that differ, not a full implementation. Proposing an alternative is the only +reason a comment runs longer. Importance isn't one. The prose of a comment, +outside any code block, has a hard limit of 600 characters. The workflow drops +a comment that goes over. + +Don't prescribe a fix in detail. Sketch it, or link to an example, and let the +author work it out. Authors feed comments to their own agents, and a +prescriptive comment gets applied verbatim whether or not it's right. + +Don't use GitHub suggestion blocks. Applying one creates a commit without a +`Signed-off-by` line, which fails DCO. + +Write plainly, as a maintainer would. The writing-style skill applies to review +comments as much as to docs. + +## What to Look For + +These are in rough order of how often maintainers raise them, which isn't the +same as how much each matters. + +### Comments and docs that don't match the code + +This is the most common category, and authors accept it nearly every time. +Look for: + +- Comments, docstrings, field descriptions and docs that describe old + behaviour, or that an agent copied from elsewhere and that still name the + wrong type, cloud or verb. +- Claims that aren't true. A label nothing reads, the wrong component + reading a field, or a stale pinned version. +- Invented rationale: a plausible reason for a decision that nothing in the + code, the issue or the design supports. +- Overclaiming. Docs describe what the project does today, not what it aims to + do. +- Comments that narrate history, such as "we used to", "this replaces", "now" + or replies to an earlier review round. History belongs in the commit + message. +- Commentary that costs the reader more than it gives them: comments three + times as long as they need to be, or that restate the code. + +### Missing "why" comments and contracts + +Where a reader would have to dig to understand something, ask for a comment +that explains why. Examples are a surprising choice, a foot-gun, a magic +number, an ignored error, or an external constraint the code works around. + +### Speculative complexity + +Push back on fields, options, knobs, helpers, abstractions and shared modules +added before anything needs them. Ask "does this need to exist?" about each +new one. Also ask whether the PR needs: + +- Guards for states that can't happen. +- Fallbacks that can't matter in practice. +- Status fields that duplicate what other state already implies. +- A special case where one code path would serve everyone. +- A helper called once, or one that does almost nothing. +- Tests that can't fail, that duplicate another case, or that only detect + change. + +### Consistency with what's already there + +New code follows the established pattern for what it's doing: test style, +naming, label keys, how a function waits on a dependency, how each cloud's +composition does the same job. When a PR adds a helper, check whether the SDK, +a dependency or the repo already has one. When a PR fixes something in one +cloud's function, check the others. + +An agent may have written an existing pattern with nobody reviewing it +closely. Say when a finding +rests on precedent alone, and don't hold a PR to a pattern that's plainly +wrong. The fix for a wrong pattern is an issue to change it everywhere. + +### Names + +A name says exactly what the thing is or does. Look for a name that drifted +when the behaviour changed, a loaded term reused for something else (Ready, +Synced, "managed"), a name that doesn't match its siblings, and grab-bag module +names like `util` or `common`. + +### Crossplane correctness + +These come up repeatedly and are easy to miss: + +- **Dropping desired state deletes things.** A composition function that + returns early, or skips composing a resource it composed before, causes + Crossplane to garbage collect that resource. A gate on a dependency must + keep composing a resource once it exists. +- **Composed resource names.** Use the SDK's naming helpers, and check names + stay within Kubernetes length limits and can't collide across replicas or + clusters. +- **Readiness.** Readiness reflects the condition the real resource sets, + never a hardcoded value. +- **Selectors inside lists** need the referenced resource's id instead. See + the composition function section of CONTRIBUTING.md. + +The crossplane-python-functions skill covers the rest of the function +contract. + +### API design + +The maintainers own API design, so raise these as questions rather than +verdicts. For XRDs, follow the krm-api-design skill: + +- Question bool fields that might need a third state. +- Mutually exclusive options want a discriminator field. +- Spec holds desired state and status holds observed state. +- Each field description reads well in `kubectl explain`. +- Enforce constraints in the schema, with CEL, rather than in code. + +### Tests + +New behaviour gets a new case in the existing table, following the Tests +section of CONTRIBUTING.md. Ask for a test where new behaviour or an error +path has none. Question a new test where the cases above it already cover +the behaviour. + +### Scope + +Unrelated changes, such as dependency bumps, drive-by refactors, new test +frameworks or new tools, belong in their own PR, and a new tool or framework +needs a decision first. + +## Language Notes + +Python, which is nearly all of Modelplane: + +- Deserialize observed resources into the generated Pydantic models rather + than chaining `.get()` calls on dicts. +- Import modules, not names from inside them. +- Fix types rather than adding `ty: ignore`. A `cast`, a `Literal` alias or + `Final` constants usually let the checker prove the value. +- Never hand-edit generated code under `schemas/python/`. + +The python-code-factoring and engineering-principles skills hold the rest. + +## The Review Summary + +The summary opens with one hedged sentence on how close the PR looks to +mergeable, without repeating any inline comment: + +- "This looks ready to me." +- "This looks close. I have a few questions." +- "I think a couple of things need another look before this merges." +- "I think we should step back and talk about the approach." + +The design reviewer's verdict weighs most in choosing it. Don't grade the PR +beyond that: leave out verdicts like "well tested" or "reads well". + +The rest of the summary explains that verdict, in a short paragraph at most, +and the whole summary stays under 1,000 characters. Findings go inline, each +in the most relevant place: + +- A finding about code the PR didn't change goes on the change that causes it. + If a comment elsewhere is now stale, comment on the change that made it + stale, and say which comment. +- For docs the PR doesn't touch, comment on the change that makes them wrong, + and link the page. +- A design question goes on the field, design doc section or code it's about. + +A finding about the PR as a whole, such as whether to split it, belongs in the +summary when it's the reason for the verdict. Findings about the commit messages +or PR description get one closing sentence there. Don't describe what the PR +does. Maintainers ask their own agents when they want that, and the PR +description is the author's job. + +Never approve or request changes. That's a maintainer's call. diff --git a/.agents/skills/reviewing-pull-requests/references/calibration.md b/.agents/skills/reviewing-pull-requests/references/calibration.md new file mode 100644 index 000000000..78007aa99 --- /dev/null +++ b/.agents/skills/reviewing-pull-requests/references/calibration.md @@ -0,0 +1,115 @@ +# Calibrating Findings + +These are real review findings on Modelplane PRs, and what happened to them. +Use them to judge a finding before posting it. Maintainers decline findings +about as often as they accept them, and for consistent reasons. + +## Posted and accepted + +Dennis on #556, which added an early return that skipped composing the cloud +cluster: + + +> **Blocking: when this gate fires on a running cluster, Crossplane deletes the +> cluster.** The early return comes before `compose_()`, and +> `check_devices_supported` sets a condition rather than a fatal result. So the +> desired state holds only the guard and namespaces, and Crossplane +> garbage-collects the cloud cluster XR, the ServingStack and their Usages. I +> ran the `GKEReady` case... + + +The comment gives the consequence and the code path that causes it, then +shows it happening in a test run. Nic raised the same problem independently on +the same PR. + +Nic on #346: + + +> I don't think this comment is true - notably nothing seems to read the +> `modelplane.ai/gpu` label. + + +A comment that claims something the code doesn't do. Short, checkable, and +explicitly not a blocker. + +Copilot on #365: + + +> cleanup() removes `${DOCKER_CONFIG:-}` on exit. If a user has DOCKER_CONFIG +> set in their environment and the script fails before it reassigns +> DOCKER_CONFIG, this can `rm -rf` the user's real Docker config directory. + + +A concrete input, a concrete failure, and a path to it. + +## Wrong + +Copilot left seven comments on #137 like this one: + + +> `$(MODELPLANE_LEADER_ADDRESS)` is shell *command substitution*, not +> environment-variable expansion. + + +The string is in a container's `command`, where Kubernetes expands `$(VAR)` +before the shell runs. The claim about an external tool was +confident, plausible and false, and each copy cost a maintainer a rebuttal. +Check how the tool behaves in this context before posting a claim like this, +and post the claim once rather than once per occurrence. + +## Declined + +A bot asked for a runtime check on a field that was about to get schema +validation: + + +> Wait isn't this guard the same thing as what #28 tracks? I don't think we +> should do it in code and also at the API level. + + +Enforce an invariant in one place. + +Asked to add migration handling for an existing cache on #406: + + +> There's nothing to migrate: Modelplane is a month old at v0.3 and I'm not +> aware of any deployment with a staged cache... I'd rather not carry migration +> machinery for a hypothetical. + + +And on #362: + + +> No one is running Modelplane v0.2 - there's no one to break, no one to +> migrate. + + +Asked to extend the test-style checker to catch patterns no test used, on #482: + + +> I'd rather not grow the checker until one actually turns up. + + +A finding that adds code needs a higher bar than one that removes it, and a +hypothetical doesn't clear it. + +A reviewer flagged that a library function took a caller-built map without +documenting whether it took ownership: + + +> This comment strikes me as an overzealous LLM... There's tons of places in +> Crossplane where the caller builds a datastructure to pass to a library +> module without the library documenting or defending against this. + + +Ordinary language semantics don't need documenting. + +## What to take from this + +- Report the evidence, not just the concern. "This could be nil" gives the + author less than "this is nil when the XR omits `spec.workers`, which the + schema allows". +- Say when a finding rests on precedent alone, since nobody may have reviewed + the precedent. +- A finding that adds guards, wrappers, migration code or defensive + documentation needs an observed failure behind it. From 125840323a1ddb28ecb9fc3d7d3c9b5bfc0f82f0 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Sat, 31 Oct 2026 23:07:21 -0700 Subject: [PATCH 03/13] Add opencode agents that review a pull request The review agent dispatches four reviewers with fresh context, for design, prose, craft and correctness, then checks their findings against the reviewing-pull-requests bar and writes what passes as markdown for a workflow to post. A re-review covers only what git range-diff shows changed since the last one. Any maintainer can run it locally with `opencode run --agent review`. The agents-vale check now lints the agents' prompts too. Signed-off-by: Nic Cope --- .opencode/agents/review-correctness.md | 36 ++++++ .opencode/agents/review-craft.md | 34 ++++++ .opencode/agents/review-design.md | 51 ++++++++ .opencode/agents/review-prose.md | 41 +++++++ .opencode/agents/review.md | 156 +++++++++++++++++++++++++ nix/checks.nix | 8 +- 6 files changed, 322 insertions(+), 4 deletions(-) create mode 100644 .opencode/agents/review-correctness.md create mode 100644 .opencode/agents/review-craft.md create mode 100644 .opencode/agents/review-design.md create mode 100644 .opencode/agents/review-prose.md create mode 100644 .opencode/agents/review.md diff --git a/.opencode/agents/review-correctness.md b/.opencode/agents/review-correctness.md new file mode 100644 index 000000000..e6e486a92 --- /dev/null +++ b/.opencode/agents/review-correctness.md @@ -0,0 +1,36 @@ +--- +description: >- + Review a pull request for correctness bugs, verified by tracing code paths, + running tests and reading upstream source. +mode: subagent +--- + +You review whether a Modelplane pull request works. Load the +reviewing-pull-requests, crossplane-python-functions and engineering-principles +skills. + +Assume the author is overconfident, and look for what's wrong. That includes +logic errors, inputs the schema allows that the code mishandles, and misuse of +the Crossplane function SDK or Kubernetes APIs. The reviewing-pull-requests +skill lists Crossplane failure modes that recur. Check whether a bug the PR +fixes also exists in the sibling code path or another cloud's function. + +Verify every finding before you report it. You can run anything. Run `nix flake +check` or a function's tests with `nix run .#test -- `, add a scratch +test case, or clone an upstream repo such as crossplane/crossplane or +crossplane/function-sdk-python to read how it really behaves. Docker works, so +a kind cluster is available when only a real API server can show the bug. You +can run any tool you need with `nix run`. The nix-tools and nix-flake skills +cover how. A finding about external behaviour cites the upstream source or docs +that show it. + +The bar is a concrete input or sequence that triggers the bug, in a deployment +someone would run. Drop hypotheticals, states the schema rules out, and +backward compatibility concerns. Modelplane is pre-1.0. Don't review prose or +style. Other reviewers cover them. + +Report each finding with its file, its line in the PR's head commit, a severity +(blocking, issue or nit), the failure, and the evidence: the input that +triggers it and the code path or test output that shows it. Leave out weak +findings, even if that leaves none. Don't edit any files in the repository. Do +scratch work under `/tmp`. diff --git a/.opencode/agents/review-craft.md b/.opencode/agents/review-craft.md new file mode 100644 index 000000000..44efa4dc1 --- /dev/null +++ b/.opencode/agents/review-craft.md @@ -0,0 +1,34 @@ +--- +description: >- + Review a pull request's code for craft: whether each addition needs to + exist, consistency with the repo, naming, tests and project style. +mode: subagent +--- + +You review the craft of a Modelplane pull request's code. Load the +reviewing-pull-requests, engineering-principles and python-code-factoring +skills, plus crossplane-python-functions for composition functions and +krm-api-design for XRDs. The Tests section of CONTRIBUTING.md governs function +tests. + +For every helper, field, option, guard, test and dependency the PR adds, ask +whether it needs to exist. Look for: + +- Speculative complexity, and guards for states the schema rules out. +- Code that solves a problem differently from how the repo already solves it, + including how the other clouds' functions do the same job. +- Code the change made redundant but left in place. +- Names that don't say what the thing is. +- Tests that can't fail, duplicate another case, or don't follow the table + pattern. +- Python project style, such as using the generated Pydantic models rather + than dicts, and importing modules rather than names. + +An existing pattern may itself be poor. Say when a finding rests on precedent +alone. Raise API design only as a question, and don't propose alternative +architectures. Don't review prose or hunt for runtime bugs. Other reviewers +cover them. + +Report each finding with its file, its line in the PR's head commit, a severity +(blocking, issue or nit), what's wrong, and the convention or code it departs +from. Leave out weak findings, even if that leaves none. Don't edit any files. diff --git a/.opencode/agents/review-design.md b/.opencode/agents/review-design.md new file mode 100644 index 000000000..51c30e7d3 --- /dev/null +++ b/.opencode/agents/review-design.md @@ -0,0 +1,51 @@ +--- +description: >- + Review a pull request's premise and design, including any design doc it + adds, against its issue and the committed designs. +mode: subagent +--- + +You review the premise and design of a Modelplane pull request. Load the +reviewing-designs and reviewing-pull-requests skills, plus krm-api-design if +the PR changes an XRD or proposes an API. + +Treat the design docs committed under `design/` on the base branch as settled. +Read the ones the PR builds on. Then look at the PR two ways. + +If the PR adds or changes a design doc, review the proposal itself with the +reviewing-designs questions. Another reviewer covers how the doc reads, so +leave its writing alone. + +Whatever the PR, check every new enum value, field or default specific to an +engine, KV transfer format, router, cloud or vendor. Would supporting the next +one need a Modelplane release? Read the principles in the committed designs, +such as `unopinionated-deployments.md`, and ask whether the PR narrows one. A +PR that fits a design's letter can still break its principles. + +For the code, ask: + +- Does it solve the problem its linked issue describes? +- Where does it depart from the committed designs or the design it adds? Does + the departure look deliberate and warranted? +- Is there a much simpler way to do it within the same design? +- Does it change behaviour, an API or a design without a linked issue? + CONTRIBUTING.md asks for an issue first for anything beyond a small fix. + +Don't review code structure, prose or correctness bugs. Other reviewers cover +them. + +Start your report with how far the PR's design looks from mergeable, in one of +these, or a sentence close to one: + +- The design looks sound. +- The design looks sound, with a few questions. +- I think some design questions need settling before this merges. +- I think we should step back and talk about the approach. + +Then report each design question with the file and line it concerns. Almost +every design question is about a specific field, section of a design doc or line +of code. Leave out the line only for a question about the PR as a whole, such as +whether to split it. Give the question, and your evidence: the committed design, +the issue or the code you're comparing against. Report only the questions a +maintainer would want asked, usually no more than three. If the design looks +sound, say so and report nothing else. Don't edit any files. diff --git a/.opencode/agents/review-prose.md b/.opencode/agents/review-prose.md new file mode 100644 index 000000000..c54d840ce --- /dev/null +++ b/.opencode/agents/review-prose.md @@ -0,0 +1,41 @@ +--- +description: >- + Review a pull request's prose: comments, docstrings, XRD field descriptions, + docs, design docs, the PR description and commit messages. +mode: subagent +--- + +You review the prose of a Modelplane pull request: whether it's true, and +whether it repays the time it takes to read. Load the reviewing-pull-requests +and writing-style skills, and the design-docs skill if the PR touches +`design/`. The writing-style skill's section on reviewing someone else's prose +is your brief, and its failure modes are your checklist. + +Review every comment, docstring, XRD field description, doc page and design doc +the PR adds or changes. Check each claim against the code, not against other +prose. Look for: + +- Claims the code doesn't support, including stale ones the PR left behind + when it changed the code they describe. +- Rationale nothing in the code, issue or design backs up. +- Comments that narrate the change or its history. +- Commentary much longer than what it says, or that restates the code. +- Places a reader needs a "why" and doesn't get one. +- Docs examples that don't match the real API. +- Design docs that miss the design-docs standard. + +Also review the PR description and commit messages against the Commit messages +and Pull requests sections of CONTRIBUTING.md. Raise these in your report as +findings with no file or line. For prose the PR didn't change but made +untrue, such as a comment or docs page elsewhere, report it against the change +that made it untrue, and say which prose is stale. + +Run Vale over any markdown the PR adds or changes outside `docs/`, with the +config the writing-style skill names. CI already lints `docs/`. + +Don't review code structure or correctness. Other reviewers cover them. + +Report each finding with its file, its line in the PR's head commit, a severity +(blocking, issue or nit), what's wrong, and the evidence: the quoted text, and +the code that contradicts it where there is some. Leave out weak findings, even +if that leaves none. Don't edit any files. diff --git a/.opencode/agents/review.md b/.opencode/agents/review.md new file mode 100644 index 000000000..7ef800d04 --- /dev/null +++ b/.opencode/agents/review.md @@ -0,0 +1,156 @@ +--- +description: >- + Review a Modelplane pull request. Runs the design, prose, craft and + correctness reviewers in parallel, then posts only the findings that hold + up. +mode: primary +--- + +You review pull requests to Modelplane on behalf of its maintainers. Your job +is to save them time by raising what they would want raised, and leaving out +what they wouldn't. Load the reviewing-pull-requests skill before you start. It +sets the bar for what to post and how to phrase it. When you need a tool you +don't have, the nix-tools skill shows how to run it with Nix. + +Your prompt identifies the PR. It may also give: + +- A context directory holding `pr.json` (the PR, its commits and linked + issues), `reviews.json`, `review-comments.json` and `issue-comments.json`. + Without one, fetch the same with `gh`. +- The commit you last reviewed. If it's given, this is a re-review. See + "Re-reviews" below. +- A directory to write the review to. Without one, print the review. + +`gh` works, with a token that can read this repo's issues, PRs and contents. +Read the issues the PR links to in full, and look for related open issues and +PRs, such as one that overlaps this PR or an issue that already settled a +question the PR raises. Give the reviewers what you find that bears on the PR. + +The working directory is a checkout of the PR's head commit. + +## 1. Understand the PR + +Read the PR description, the linked issues, any design doc under `design/` the +change builds on, and every existing review comment and thread. Work out the +base with `git merge-base` against the PR's base branch, then read the whole +diff. + +Work out what the PR does, why, and where it departs from its issue or design. +The reviewers' findings are only as good as your check of them, and that check +needs this understanding. Don't write it into the review. + +### Re-reviews + +A re-review covers only what changed since your last review. The workflow +fetches the commit you last reviewed into the checkout. Authors usually fold +fixes into their original commits and force-push, so compare the two versions +of the PR rather than the commits between them: + +```bash +git range-diff $(git merge-base origin/ ).. $(git merge-base origin/ HEAD)..HEAD +``` + +`` is the PR's base branch, `baseRefName` in `pr.json`. A backport PR +targets a release branch rather than `main`. The range-diff pairs each commit +you reviewed with its new version and shows how it changed. A rebase onto a +newer base drops out. Give the reviewers the range-diff as what to review, +alongside the existing threads, so they can tell which earlier comments the +changes address. + +Raise nothing about code that hasn't changed since your last review. The +workflow drops any inline comment on a line that hasn't changed since then. A +change can still break code it didn't touch, such as a caller of a function +whose behaviour changed. Put a finding like that on the change that breaks it. + +The summary of a re-review covers only what changed, too. Don't restate or +remind the author of earlier findings, whether or not they look addressed: +the earlier review and its threads already say them. If the changes raise +nothing new, the summary says so in one sentence. + +## 2. Run the reviewers + +Dispatch the `review-design`, `review-prose`, `review-craft` and +`review-correctness` subagents in parallel, as four calls in one message. Run +them in the foreground. A headless run ends when your turn does, killing any +background subagent before it reports. Give each the PR number, the diff range +to review, the context directory if you have one, and the PR description and +linked issues as the statement of what the PR sets out to do. Don't give them +your own assessment of the PR, so they form their own. + +## 3. Reconcile + +You decide what's posted. Treat each reviewer's findings as claims to check, +not work to forward: + +- Drop any finding that fails the bar in the reviewing-pull-requests skill. +- Check the evidence yourself for anything you'll post as blocking or as a + bug. Read the code path, or rerun what the reviewer ran. +- Drop anything an existing review thread already raised, answered or + declined. +- Merge findings that are one issue in several places into one comment that + says "(here and elsewhere)". +- Post every finding that passes the skill's tests for posting. How many + that is depends on the PR: a small PR often gets none, and a large one can + get fifteen. The workflow refuses a review with more than 25 comments. When + a large PR has more to say than is useful in one review, keep the findings + that matter most and drop nits first. +- Keep every point the design reviewer makes. Don't merge one question into + another or drop its evidence, above all where it cites a principle from a + committed design. Each still goes where any finding goes: inline on the line + it concerns, and in the summary only if no line fits. The length rules for + comments apply to design questions too. +- If the design reviewer thinks the design needs settling, or the approach + rethinking, hold back the other reviewers' findings on naming, tests and + style. They're wasted if the design changes. Keep correctness findings that + would matter whatever the design becomes. + +## 4. Write the review + +Load the writing-style skill before you write, and follow it. Don't stop to ask +questions, since nobody is there to answer. You post as the project's review +bot. The skill's rules about writing under a person's name don't apply. Write +the summary and each comment as the reviewing-pull-requests skill describes. Its +section on phrasing matters most: ask, explain and suggest for anything about +correctness or design, even when the reviewers stated it as fact, and be direct +only about style. The summary's opening sentence matches the design reviewer's +verdict, and mustn't soften it. Attach every comment to a line the PR changed. +That's the line a finding is about, or for code the PR didn't change, the change +that causes the problem. Keep the summary to the verdict and why, under 1,000 +characters, as the skill describes. + +Write the review as markdown files in the directory you're given. A linter +checks each one as you write it. Don't hard-wrap them. GitHub shows a newline in +a review comment as a line break. Write the summary to `summary.md`, and each +inline comment to its own file under `comments/`, named for its order, such as +`comments/01.md`. A comment file starts with front matter giving its file and +its line in the PR's head commit: + +```markdown +--- +path: functions/compose-model-cache/function/fn.py +line: 42 +--- + +Won't returning here delete the cache's PVC? It looks like Crossplane garbage +collects composed resources that drop out of the desired state. Could the gate +keep composing the PVC once it exists? +``` + +Fix every error the linter reports on these files. When you've written them +all, run `vale --config .opencode/vale/.vale.ini` over the directory, after +`vale sync --config .opencode/vale/.vale.ini` if its styles are missing, and fix +what it reports too. + +## 5. De-slop the review + +Vale catches phrasing, not padding, invented reasons or a comment that argues +past its point. Dispatch a subagent with fresh context to catch those. Give it +the writing-style skill's `references/de-slop-review.md` and the review +directory. Tell it the audience is the PR's author and the maintainers, the +purpose is to tell them what to change and why, and the reviewing-pull-requests +skill's phrasing rules replace the brief's design-doc examples. It skips the +brief's structure check, which is for design docs. + +Fix what it reports in the wording. Don't add, drop or change findings because +of it, since it reviews prose, not substance. Then run Vale over the directory +again and fix what it reports. diff --git a/nix/checks.nix b/nix/checks.nix index 1ae843ecf..e166a07ef 100644 --- a/nix/checks.nix +++ b/nix/checks.nix @@ -38,9 +38,9 @@ in # docs-site repo, which builds and link-checks it where it deploys. docs-vale = docs.vale; - # Lint the skills under .agents with the ai-tells rules and a few Google - # ones, using the Vale config in .opencode/vale. Reuses the docs check's - # synced style packages. + # Lint the skills and agent prompts under .agents and .opencode with the + # Vale config the review agent lints its own output with: the ai-tells rules + # and a few Google ones. Reuses the docs check's synced style packages. agents-vale = pkgs.runCommand "modelplane-agents-vale" { @@ -54,7 +54,7 @@ in cp ${self}/.opencode/vale/.vale.ini .vale.ini mkdir styles cp -r --no-preserve=mode ${docs.valeStyles}/* styles/ - find ${self}/.agents -name '*.md' -print0 | \ + find ${self}/.agents ${self}/.opencode/agents -name '*.md' -print0 | \ xargs -0 --no-run-if-empty \ vale --config="$PWD/.vale.ini" mkdir -p $out From 05b364664f5fd6e840438c84c67d1c51bf181009 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Sat, 31 Oct 2026 23:06:43 -0700 Subject: [PATCH 04/13] Add a Firecracker microVM to run the review agent in The review agent runs the PR's code, so it needs a sandbox, and a container shares the runner's kernel and can't run Docker for a kind cluster. This flake builds a microVM with opencode, Vale, gh, Nix and Docker. It shares no files with the host: its job arrives as a tar on a read-only disk, and it writes its output as a tar to another. It's a flake of its own so the project's flake doesn't depend on microvm.nix. Signed-off-by: Nic Cope --- hack/review-vm/flake.lock | 83 ++++++++++++ hack/review-vm/flake.nix | 270 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 353 insertions(+) create mode 100644 hack/review-vm/flake.lock create mode 100644 hack/review-vm/flake.nix diff --git a/hack/review-vm/flake.lock b/hack/review-vm/flake.lock new file mode 100644 index 000000000..a20b37ffc --- /dev/null +++ b/hack/review-vm/flake.lock @@ -0,0 +1,83 @@ +{ + "nodes": { + "microvm": { + "inputs": { + "nixpkgs": [ + "nixpkgs" + ], + "spectrum": "spectrum" + }, + "locked": { + "lastModified": 1793281488, + "narHash": "sha256-ErGIqtoWEX9KKC8NFtJDx2pGGG02Cr6LGxrvzLd2sqA=", + "owner": "microvm-nix", + "repo": "microvm.nix", + "rev": "b7bd556974fa8a171b5b639200e81d52244dfde4", + "type": "github" + }, + "original": { + "owner": "microvm-nix", + "repo": "microvm.nix", + "rev": "b7bd556974fa8a171b5b639200e81d52244dfde4", + "type": "github" + } + }, + "nixpkgs": { + "locked": { + "lastModified": 1793410479, + "narHash": "sha256-HzgjxrLY4AEc0044ZuM5CHbLSWLZgDlay1idmzvvpJs=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "fa1373130d5a21c040a47c5dbe1913d59ed91808", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixos-26.05", + "repo": "nixpkgs", + "type": "github" + } + }, + "nixpkgs-unstable": { + "locked": { + "lastModified": 1793341285, + "narHash": "sha256-Y1CiZ4SoBUjLQyQM/pY+rS2dXD3CHLPstF0lmOc73D4=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "1c89083e451e891152365b799595cf7790a43325", + "type": "github" + }, + "original": { + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "1c89083e451e891152365b799595cf7790a43325", + "type": "github" + } + }, + "root": { + "inputs": { + "microvm": "microvm", + "nixpkgs": "nixpkgs", + "nixpkgs-unstable": "nixpkgs-unstable" + } + }, + "spectrum": { + "flake": false, + "locked": { + "lastModified": 1786032750, + "narHash": "sha256-+Ba/kIn5BjvV7PN8bWOo7RoqgTY/Yv2M7qeMf3wS2Ag=", + "ref": "refs/heads/main", + "rev": "77d2389874bb092b8dbf05ecc5b1030372e80e98", + "revCount": 1417, + "type": "git", + "url": "https://spectrum-os.org/git/spectrum" + }, + "original": { + "type": "git", + "url": "https://spectrum-os.org/git/spectrum" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/hack/review-vm/flake.nix b/hack/review-vm/flake.nix new file mode 100644 index 000000000..2a4e194b5 --- /dev/null +++ b/hack/review-vm/flake.nix @@ -0,0 +1,270 @@ +# Copyright 2026 The Modelplane Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# The Firecracker microVM the review workflow runs the review agent in. It's a +# flake of its own so the project's flake doesn't depend on microvm.nix. +# +# The VM shares nothing with the runner. Its job arrives as a tar on a +# read-only disk: the PR's checkout, the PR's context and the agent's prompt. +# It writes the review and the agent's transcript as a tar to a second disk, +# which the runner reads without mounting. Its only network is a tap device the +# runner firewalls, which reaches the internet and the model proxy at +# 192.168.100.1. +{ + description = "The microVM the review agent runs in"; + + inputs = { + nixpkgs.url = "github:NixOS/nixpkgs/nixos-26.05"; + # opencode, Vale and vale-ls come from unstable. 26.05 ships an older major + # version of opencode, and a Vale that ignores the relative StylesPath in + # the repo's Vale config. + nixpkgs-unstable.url = "github:NixOS/nixpkgs/1c89083e451e891152365b799595cf7790a43325"; + microvm = { + url = "github:microvm-nix/microvm.nix/b7bd556974fa8a171b5b639200e81d52244dfde4"; + inputs.nixpkgs.follows = "nixpkgs"; + }; + }; + + outputs = + { + nixpkgs, + nixpkgs-unstable, + microvm, + ... + }: + { + nixosConfigurations.agent = nixpkgs.lib.nixosSystem { + system = "x86_64-linux"; + modules = [ + microvm.nixosModules.microvm + ( + { pkgs, lib, ... }: + let + unstable = nixpkgs-unstable.legacyPackages.${pkgs.stdenv.hostPlatform.system}; + + # opencode 2 has no language server support of its own. This + # plugin adds it, so the agent sees Vale's diagnostics on every + # review file it writes. + lspPlugin = pkgs.fetchFromGitHub { + owner = "christensenjairus"; + repo = "opencode-v2-lsp-support"; + rev = "4a17bc8379e572ab104ec26b55f92ee9b51dce37"; + hash = "sha256-vSlq1GVtqEdqYBVZU3+UyCH5Ke4nQwY1Y9VWcoUWiu8="; + }; + + opencodeConfig = builtins.toJSON { + # The review script adds the model and its provider, which the + # workflow passes in model.json. + autoupdate = false; + plugins = [ "${lspPlugin}" ]; + lsp.vale = { + command = [ "vale-ls" ]; + extensions = [ ".md" ]; + initialization = { + configPath = "/work/pr/.opencode/vale/.vale.ini"; + installVale = false; + syncOnStartup = false; + }; + }; + }; + in + { + system.stateVersion = "26.05"; + + microvm = { + hypervisor = "firecracker"; + # Firecracker needs one or an even number of vCPUs with SMT. + vcpu = 4; + mem = 12288; + + interfaces = [ + { + type = "tap"; + id = "vm0"; + mac = "02:00:00:00:00:01"; + } + ]; + + # Firecracker attaches the store disk first, then these in + # order, so they're vdb through vdf. The runner creates them + # in the working directory it starts the VM from. + writableStoreOverlay = "/nix/.rw-store"; + volumes = [ + { + image = "store.img"; + mountPoint = "/nix/.rw-store"; + size = 40960; + } + { + image = "docker.img"; + mountPoint = "/var/lib/docker"; + size = 20480; + } + { + image = "work.img"; + mountPoint = "/work"; + size = 20480; + } + { + image = "in.tar"; + mountPoint = null; + autoCreate = false; + readOnly = true; + } + { + image = "out.tar"; + mountPoint = null; + autoCreate = false; + } + ]; + }; + + networking = { + hostName = "agent"; + useNetworkd = true; + useDHCP = false; + enableIPv6 = false; + }; + systemd.network.networks."10-vm0" = { + matchConfig.MACAddress = "02:00:00:00:00:01"; + address = [ "192.168.100.2/30" ]; + routes = [ { Gateway = "192.168.100.1"; } ]; + dns = [ + "1.1.1.1" + "8.8.8.8" + ]; + }; + + nix.settings.experimental-features = [ + "nix-command" + "flakes" + ]; + virtualisation.docker.enable = true; + + users.users.root.password = ""; + services.getty.autologinUser = "root"; + + # Unpack the job, run the agent, write its output back as a tar, + # then reboot. Firecracker has no power-off device, so a halted + # guest leaves it running. It exits on a guest reboot. + systemd.services.review = { + wantedBy = [ "multi-user.target" ]; + after = [ + "network-online.target" + "docker.service" + ]; + wants = [ "network-online.target" ]; + path = [ + unstable.opencode + pkgs.bash + pkgs.coreutils + pkgs.curl + pkgs.docker + pkgs.findutils + pkgs.gh + pkgs.git + pkgs.gnugrep + pkgs.gnutar + pkgs.jq + pkgs.kind + pkgs.kubectl + pkgs.nix + pkgs.procps + pkgs.ripgrep + unstable.vale + unstable.vale-ls + ]; + environment = { + HOME = "/root"; + TMPDIR = "/work/tmp"; + OPENCODE_CONFIG_CONTENT = opencodeConfig; + }; + serviceConfig = { + Type = "oneshot"; + StandardOutput = "journal+console"; + StandardError = "journal+console"; + }; + script = '' + set +e + mkdir -p /work/tmp /work/out + tar -xf /dev/vde -C /work + # A token that can only read this repo's contents, PRs and + # issues, all of which are public, so the agent can use gh. + GH_TOKEN=$(cat /work/gh-token) + export GH_TOKEN + rm /work/gh-token + cd /work/pr + git config --global --add safe.directory /work/pr + mkdir -p _output/review + vale sync --config .opencode/vale/.vale.ini + # The console, which the workflow's log shows, gets opencode's + # own readable output. The transcript is each session's + # export, the agent's and every subagent's, with their costs. + # The agent reaches the model through the runner's proxy, which + # replaces the API key. + model=$(jq -r .model /work/model.json) + case $(jq -r .api /work/model.json) in + openai) + config=$(jq -n --arg model "$model" '{ + model: ("inference/" + $model), + provider: {inference: { + npm: "@ai-sdk/openai-compatible", + options: {baseURL: "http://192.168.100.1:8787/v1", apiKey: "unused"}, + models: {($model): {}} + }} + }') + ;; + *) + config=$(jq -n --arg model "$model" '{ + model: ("anthropic/" + $model), + provider: {anthropic: { + options: {baseURL: "http://192.168.100.1:8787/v1", apiKey: "unused"} + }} + }') + ;; + esac + OPENCODE_CONFIG_CONTENT=$(jq -c --argjson config "$config" '. + $config' <<< "$OPENCODE_CONFIG_CONTENT") + export OPENCODE_CONFIG_CONTENT + opencode run --standalone --auto --agent review "$(cat /work/prompt)" + status=$? + mkdir -p /work/out/transcript + pending=$(opencode session list --standalone --format json | jq -r '.[].id') + while [ -n "$pending" ]; do + next="" + for id in $pending; do + out=/work/out/transcript/$id.json + [ -e "$out" ] && continue + opencode session export --standalone "$id" > "$out" + next="$next $(grep -o 'subagent sessionID=\\"ses_[A-Za-z0-9]*' "$out" | grep -o 'ses_[A-Za-z0-9]*' | sort -u)" + done + pending=$next + done + # opencode run exits 1 when any model request fails, even one + # it retried and recovered from. The root session's outcome + # says whether the review finished. + outcome=$(jq -r 'select(.info.parentID == null) | .info.outcome' /work/out/transcript/*.json) + if [ "$outcome" = succeeded ]; then status=0; fi + echo "$status" > /work/out/exit + cp -r _output/review /work/out/review + tar -cf /dev/vdf -C /work/out . + sync + ${lib.getExe' pkgs.systemd "systemctl"} reboot + ''; + }; + } + ) + ]; + }; + }; +} From c6f0f9f6fdee6698b60d5f88932e4cb31786f02c Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Sat, 31 Oct 2026 23:06:43 -0700 Subject: [PATCH 05/13] Review pull requests with an agent in CI PRs from org members are reviewed when they're opened or marked ready, and a /review comment from anyone with write access reviews any PR, or re-reviews it after new pushes. Every PR comes from a fork, so the workflow runs on pull_request_target, and the agent runs the PR's code in the microVM. A Caddy proxy on the runner adds the Anthropic key to its model requests and forwards only POST /v1/messages, and a firewall limits the VM to the proxy and the internet. The VM gets only the job's read-only token. A second job with no model reads the review from the VM's output tar, validates it with post_review.py and posts it as a GitHub App. On a re-review, the script only posts comments on lines that changed since the last one. GitHub doesn't officially support KVM on its runners, so review-retry.yml re-runs a review that landed on a runner without it. Signed-off-by: Nic Cope --- .github/workflows/commands.yml | 48 ++++ .github/workflows/review-retry.yml | 33 +++ .github/workflows/review.yml | 344 +++++++++++++++++++++++++++++ hack/post_review.py | 323 +++++++++++++++++++++++++++ 4 files changed, 748 insertions(+) create mode 100644 .github/workflows/review-retry.yml create mode 100644 .github/workflows/review.yml create mode 100644 hack/post_review.py diff --git a/.github/workflows/commands.yml b/.github/workflows/commands.yml index 1ad283062..beb21b754 100644 --- a/.github/workflows/commands.yml +++ b/.github/workflows/commands.yml @@ -36,3 +36,51 @@ jobs: with: github_token: ${{ secrets.GITHUB_TOKEN }} github_workspace: ${{ github.workspace }} + + # Pins the PR's head commit when the comment arrives, so a push after a + # maintainer asks for a review of someone else's PR isn't what gets reviewed. + review-command: + permissions: + pull-requests: write + runs-on: ubuntu-24.04 + # Only a new comment. The slash command action checks the commenter's + # permission on created and edited comments, and lets a deleted one through + # unchecked. + if: >- + vars.REVIEW_APP_ID != '' && + github.event.action == 'created' && + github.event.issue.pull_request && + startsWith(github.event.comment.body, '/review') + outputs: + sha: ${{ steps.head.outputs.sha }} + steps: + - name: Extract Command + uses: xt0rted/slash-command-action@bf51f8f5f4ea3d58abc7eca58f77104182b23e88 # v2 + with: + repo-token: ${{ secrets.GITHUB_TOKEN }} + command: review + reaction: "true" + reaction-type: "eyes" + allow-edits: "false" + permission-level: write + + - name: Find the PR's head commit + id: head + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR: ${{ github.event.issue.number }} + run: | + sha=$(gh api "repos/$GITHUB_REPOSITORY/pulls/$PR" --jq .head.sha) + echo "sha=$sha" >> "$GITHUB_OUTPUT" + + review: + needs: review-command + permissions: + contents: read + pull-requests: read + issues: read + uses: ./.github/workflows/review.yml + with: + pr: ${{ github.event.issue.number }} + sha: ${{ needs.review-command.outputs.sha }} + secrets: inherit diff --git a/.github/workflows/review-retry.yml b/.github/workflows/review-retry.yml new file mode 100644 index 000000000..b6ace4445 --- /dev/null +++ b/.github/workflows/review-retry.yml @@ -0,0 +1,33 @@ +name: Review Retry + +# GitHub's x86 runners expose KVM, but don't guarantee it, and the review VM +# can't start on a runner without it. When a review fails for that reason, re-run +# its failed jobs, which usually get a different runner. Two retries at most. +on: + workflow_run: + workflows: [Review, Comment Commands] + types: [completed] + +permissions: {} + +jobs: + retry: + if: >- + vars.REVIEW_APP_ID != '' && + github.event.workflow_run.conclusion == 'failure' && + github.event.workflow_run.run_attempt < 3 + runs-on: ubuntu-24.04 + permissions: + actions: write + steps: + - name: Re-run a review that had no KVM + env: + GH_TOKEN: ${{ github.token }} + RUN: ${{ github.event.workflow_run.id }} + ATTEMPT: ${{ github.event.workflow_run.run_attempt }} + run: | + failed=$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/$RUN/attempts/$ATTEMPT/jobs" \ + --jq '[.jobs[].steps[] | select(.name == "Allow the runner user to use KVM" and .conclusion == "failure")] | length') + if [ "$failed" -gt 0 ]; then + gh run rerun "$RUN" --failed --repo "$GITHUB_REPOSITORY" + fi diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml new file mode 100644 index 000000000..0eeea2117 --- /dev/null +++ b/.github/workflows/review.yml @@ -0,0 +1,344 @@ +name: Review + +# An agent reviews pull requests from org members when they're opened or marked +# ready for review. A maintainer can review any other PR, or re-review one after +# new pushes, by commenting /review (see commands.yml). +# +# pull_request_target runs with secrets even for PRs from forks, which every PR +# is. So the agent runs the PR's code in a Firecracker microVM that holds no +# credential that can do harm: a read-only GitHub token, and no model API key. +# A proxy on the runner adds the key to the agent's model requests. The VM writes its review to a disk as +# a tar, and a separate job with no model in it validates and posts the review. +on: + pull_request_target: + types: [opened, ready_for_review] + workflow_call: + inputs: + pr: + description: The number of the PR to review. + type: string + required: true + sha: + description: >- + The head commit to review. A /review comment pins the PR's head + commit when its job starts. + type: string + required: true + +permissions: {} + +concurrency: + group: review-${{ inputs.pr || github.event.pull_request.number }} + cancel-in-progress: false + +env: + PR: ${{ inputs.pr || github.event.pull_request.number }} + SHA: ${{ inputs.sha || github.event.pull_request.head.sha }} + + CADDY_IMAGE: caddy@sha256:36c24ad91a7ff7bc4c87fe22bfa8df3c4b18da0773d73fc12ee2c52b98c0bb7e + +jobs: + # Review automatically only PRs that org members open. The event's + # author_association reports a private member as a CONTRIBUTOR, so ask the + # org instead. + # + # Every job needs the reviewer's GitHub App, so a repo without one, such as + # a contributor's fork, never runs them. + author: + if: >- + vars.REVIEW_APP_ID != '' && + github.event_name == 'pull_request_target' && + !github.event.pull_request.draft + runs-on: ubuntu-24.04 + permissions: {} + outputs: + member: ${{ steps.member.outputs.member }} + steps: + - name: Mint a token to read org membership + if: github.event.repository.owner.type == 'Organization' + id: token + uses: actions/create-github-app-token@fee1f7d63c2ff003460e3d139729b119787bc349 # v2.2.2 + with: + app-id: ${{ vars.REVIEW_APP_ID }} + private-key: ${{ secrets.REVIEW_APP_KEY }} + owner: ${{ github.repository_owner }} + permission-members: read + + - name: Check the author is an org member + id: member + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + AUTHOR: ${{ github.event.pull_request.user.login }} + OWNER_TYPE: ${{ github.event.repository.owner.type }} + run: | + # A repo a person owns, such as a maintainer's fork used to try the + # reviewer, has no members. There the owner counts as one. + if [ "$OWNER_TYPE" = User ]; then + if [ "$AUTHOR" = "$GITHUB_REPOSITORY_OWNER" ]; then member=true; else member=false; fi + echo "member=$member" >> "$GITHUB_OUTPUT" + elif gh api --silent "orgs/$GITHUB_REPOSITORY_OWNER/members/$AUTHOR"; then + echo "member=true" >> "$GITHUB_OUTPUT" + else + echo "member=false" >> "$GITHUB_OUTPUT" + fi + + review: + # A /review comment skips the author job, having already checked that the + # commenter can write. + needs: author + if: >- + !cancelled() && + vars.REVIEW_APP_ID != '' && ( + github.event_name == 'issue_comment' || + needs.author.outputs.member == 'true' + ) + runs-on: ubuntu-24.04 + timeout-minutes: 60 + outputs: + last: ${{ steps.context.outputs.last }} + permissions: + # The runner uses these to fetch the PR's context, and the agent to read + # issues and other PRs with gh. All are read-only. + contents: read + pull-requests: read + issues: read + + steps: + - name: Checkout the reviewer + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + path: reviewer + persist-credentials: false + + - name: Checkout the PR + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + ref: ${{ env.SHA }} + path: pr + fetch-depth: 0 + persist-credentials: false + + # Everything opencode reads as config or instructions comes from main, as + # does CONTRIBUTING.md, which the skills defer to. That stops a PR + # editing its own review instructions. PR code the agent runs can still + # rewrite them mid-review, which the design accepts. + - name: Install the reviewer's config from main + run: | + for f in .opencode .agents .claude opencode.json opencode.jsonc AGENTS.md CLAUDE.md CONTRIBUTING.md; do + rm -rf "pr/$f" + if [ -e "reviewer/$f" ]; then cp -r "reviewer/$f" "pr/$f"; fi + done + + - name: Fetch the PR's context + id: context + env: + GH_TOKEN: ${{ github.token }} + run: | + mkdir context + gh pr view "$PR" --repo "$GITHUB_REPOSITORY" \ + --json number,title,body,author,baseRefName,headRefOid,commits,closingIssuesReferences,files \ + > context/pr.json + gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews" > context/reviews.json + gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR/comments" > context/review-comments.json + gh api --paginate "repos/$GITHUB_REPOSITORY/issues/$PR/comments" > context/issue-comments.json + + # The last commit a bot review covered, from the marker post_review.py puts + # in each review it posts. + last=$(jq -rs 'add | map(select(.user.type == "Bot")) | map(.body | capture("").sha) | last // ""' context/reviews.json) + echo "LAST=$last" >> "$GITHUB_ENV" + echo "last=$last" >> "$GITHUB_OUTPUT" + + # A re-review compares the PR with the version it last reviewed, + # which is usually gone from the branch after a force-push. GitHub + # still serves it by its hash. Without it there's no telling what + # changed, and reviewing the whole PR again would reopen what the + # last review covered, so stop. + if [ -n "$last" ] && ! git -C pr fetch -q origin "$last"; then + echo "::error::Can't fetch $last, the commit the last review covered, so can't tell what changed since." + exit 1 + fi + + - name: Allow the runner user to use KVM + run: | + echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules + sudo udevadm control --reload-rules + sudo udevadm trigger --name-match=kvm + # GitHub's x86 runners expose KVM in practice, but don't guarantee + # it, and a runner without it can't start the VM. + if ! test -w /dev/kvm; then + ls -l /dev/kvm || true + echo "::error::This runner has no usable /dev/kvm, so it can't run the review VM. The Review Retry workflow re-runs the job, up to twice." + exit 1 + fi + + - name: Install Nix + uses: cachix/install-nix-action@v31 + + # The VM reaches the runner only on the proxy's port, and reaches the + # internet through NAT. Private, link-local and Azure platform addresses + # stay out of reach, including the runner's own. + - name: Create the VM's network + run: | + sudo ip tuntap add vm0 mode tap user "$USER" + sudo ip addr add 192.168.100.1/30 dev vm0 + sudo ip link set vm0 up + sudo sysctl -qw net.ipv4.ip_forward=1 + uplink=$(ip route show default | awk '{print $5; exit}') + sudo iptables -t nat -A POSTROUTING -s 192.168.100.0/30 -o "$uplink" -j MASQUERADE + sudo iptables -I FORWARD 1 -i vm0 -o "$uplink" -j ACCEPT + sudo iptables -I FORWARD 1 -o vm0 -m conntrack --ctstate ESTABLISHED,RELATED -j ACCEPT + for net in 10.0.0.0/8 172.16.0.0/12 192.168.0.0/16 100.64.0.0/10 169.254.0.0/16 168.63.129.16/32; do + sudo iptables -I FORWARD 1 -i vm0 -d "$net" -j DROP + done + sudo iptables -I INPUT 1 -i vm0 -j DROP + sudo iptables -I INPUT 1 -i vm0 -p tcp --dport 8787 -j ACCEPT + # The VM runs as root, so it can turn IPv6 back on. Drop all of it. + sudo sysctl -qw net.ipv6.conf.vm0.disable_ipv6=1 + sudo ip6tables -I INPUT 1 -i vm0 -j DROP + sudo ip6tables -I FORWARD 1 -i vm0 -j DROP + + # The model comes from repo variables. REVIEW_MODEL_API is anthropic (the + # default) or openai, for an OpenAI-compatible chat completions API. + # REVIEW_MODEL_URL is the API's base URL, and REVIEW_MODEL the model. The + # proxy forwards only the one endpoint the agent needs, so the agent can't + # use the key for anything else the key can reach. + - name: Start the model proxy + env: + MODEL_API_KEY: ${{ secrets.REVIEW_MODEL_API_KEY }} + MODEL_API: ${{ vars.REVIEW_MODEL_API || 'anthropic' }} + MODEL_URL: ${{ vars.REVIEW_MODEL_URL || 'https://api.anthropic.com/v1' }} + run: | + case "$MODEL_API" in + anthropic) + endpoint=/v1/messages + auth='x-api-key {env.MODEL_API_KEY}' + ;; + openai) + endpoint=/v1/chat/completions + auth='Authorization "Bearer {env.MODEL_API_KEY}"' + ;; + *) + echo "::error::REVIEW_MODEL_API must be anthropic or openai, not $MODEL_API." + exit 1 + ;; + esac + # The agent calls the proxy at /v1. The proxy swaps that for the + # path of the API's base URL, such as /openai/v1. + origin=$(printf '%s' "$MODEL_URL" | sed -E 's#^(https?://[^/]+).*#\1#') + host=${origin#*://} + prefix=${MODEL_URL#"$origin"} + prefix=${prefix%/} + cat > Caddyfile < prompt + printf '%s' "$GH_TOKEN" > gh-token + jq -n --arg api "$MODEL_API" --arg model "$MODEL" '{api: $api, model: $model}' > model.json + + sudo mkdir -p /mnt/vm + sudo chown "$USER" /mnt/vm + tar -cf /mnt/vm/in.tar pr context prompt gh-token model.json + rm gh-token + truncate -s 1G /mnt/vm/out.tar + + - name: Review + working-directory: /mnt/vm + run: timeout 50m "$GITHUB_WORKSPACE/vm/bin/microvm-run" < /dev/null + + # The output disk is a tar the VM wrote. Upload it as it is: nothing + # extracts or mounts it on a runner. + - name: Upload the VM's output + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: review + path: /mnt/vm/out.tar + if-no-files-found: ignore + + # A /review skips the author job. Without an explicit condition, GitHub + # would skip this job too, since it depends on a job that depends on one + # that was skipped. + post: + needs: review + if: ${{ !cancelled() && needs.review.result == 'success' }} + runs-on: ubuntu-24.04 + permissions: {} + + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + + - name: Download the review + uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 + with: + name: review + + # A re-review may only comment on lines that changed since the last + # review, which post_review.py works out by diffing the two commits. + - name: Fetch the commits a re-review compares + if: needs.review.outputs.last != '' + env: + LAST: ${{ needs.review.outputs.last }} + run: git fetch -q --depth 1 origin "$LAST" "$SHA" + + # A GitHub App posts reviews, so they come from the reviewer rather than + # github-actions. Its token can write pull requests in this repo only. + - name: Mint a token for the reviewer + id: token + uses: actions/create-github-app-token@fee1f7d63c2ff003460e3d139729b119787bc349 # v2.2.2 + with: + app-id: ${{ vars.REVIEW_APP_ID }} + private-key: ${{ secrets.REVIEW_APP_KEY }} + permission-pull-requests: write + permission-contents: read + + - name: Post the review + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + LAST: ${{ needs.review.outputs.last }} + run: | + python3 hack/post_review.py --repo "$GITHUB_REPOSITORY" --pr "$PR" --sha "$SHA" \ + ${LAST:+--since "$LAST" --git-dir .git} out.tar diff --git a/hack/post_review.py b/hack/post_review.py new file mode 100644 index 000000000..43c2521b1 --- /dev/null +++ b/hack/post_review.py @@ -0,0 +1,323 @@ +#!/usr/bin/env python3 +# Copyright 2026 The Modelplane Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Post a review the review agent wrote as a GitHub pull request review. + +The agent runs in a VM that writes its output as a tar: the review under +review/, as summary.md and one file per inline comment under review/comments/, +each starting with front matter that gives its path and line. The agent runs +the PR's code, so its output is untrusted. This reads the tar's members in +memory, without extracting it, checks the review's shape and size, drops any +comment that fails a check, and always posts a COMMENT review: the reviewer +never approves or requests changes. GitHub rejects a whole review if one +comment targets a line outside the diff, so comments on such lines move into +the summary instead. The summary ends with a marker recording the commit the +review covered, which the next review reads to review only what's new. The +summary keeps to its 1,000 character limit by dropping later paragraphs. A +review from an agent that didn't exit cleanly may be incomplete, so it gets no +marker. + +A re-review passes --since, the commit the last review covered. Its inline +comments may then only target lines the PR adds that also changed since that +commit, so a re-review can't reopen the parts of the PR it already reviewed. +Comments on other lines are dropped rather than moved into the summary. +""" + +import argparse +import json +import pathlib +import re +import subprocess +import sys +import tarfile +import typing + +# The bar decides how many comments a review has. This only stops a runaway. +MAX_COMMENTS = 25 +MAX_SUMMARY = 20_000 +MAX_COMMENT = 4_000 +# The skill limits the summary to the verdict and why, under this many +# characters. Paragraphs past it are dropped. +MAX_SUMMARY_PROSE = 1_000 +# GitHub limits a review body to 65,536 characters. This leaves room for the +# marker. +MAX_BODY = 65_000 + +# The skill limits a comment's prose, outside any code block, to this many +# characters. A sketch of an alternative can run longer. +MAX_PROSE = 600 + +# The review's last line is a marker recording the commit it covered, and the +# next review reads it to scope itself. So the agent can't write one, the +# review escapes every HTML comment opener in what the agent wrote. +MARKER_FORMAT = "" +FRONT_MATTER = re.compile(r"\A---\n(?P.*?)\n---\n(?P.*)\Z", re.DOTALL) +CODE_BLOCK = re.compile(r"^```.*?^```", re.DOTALL | re.MULTILINE) +HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(?P\d+)(?:,(?P\d+))? @@") + + +class Comment(typing.NamedTuple): + """An inline review comment.""" + + path: str + line: int + body: str + + +class Review(typing.NamedTuple): + """A review summary and its inline comments.""" + + summary: str + comments: list[Comment] + # Whether the agent exited cleanly. A review from a run that didn't may be + # incomplete. + complete: bool + + +# Lines that start a new markdown block, rather than continuing a paragraph. +BLOCK_START = re.compile(r"^\s*([-*+] |\d+[.)] |>|#|\||<)") + + +def unwrap(text: str) -> str: + """text with hard-wrapped paragraphs and list items joined into single lines. + + GitHub renders a newline in a review comment as a line break, so text the + agent wrapped at 80 columns would show ragged. Code blocks keep their lines. + """ + out: list[str] = [] + fenced = False + for line in text.split("\n"): + if line.lstrip().startswith("```"): + fenced = not fenced + out.append(line) + continue + joinable = out and out[-1].strip() and not out[-1].lstrip().startswith("```") + if not fenced and joinable and line.strip() and not BLOCK_START.match(line): + out[-1] = out[-1].rstrip() + " " + line.strip() + continue + out.append(line) + return "\n".join(out) + + +def unmark(text: str) -> str: + """text with every HTML comment opener escaped, so it renders as written + and can't pass for the review's marker.""" + return text.replace(" +> Should we just use an EPP all the time? It'd be vestigial for the single pod +> case, but presumably simplify the system. + ### Will the API hold up? diff --git a/.agents/skills/reviewing-pull-requests/SKILL.md b/.agents/skills/reviewing-pull-requests/SKILL.md index 5b80f60ee..cead7b9b6 100644 --- a/.agents/skills/reviewing-pull-requests/SKILL.md +++ b/.agents/skills/reviewing-pull-requests/SKILL.md @@ -80,21 +80,28 @@ Offering options matters for the same reason. An agent handed one fix applies it as written. Handed a few, or told the reviewer doesn't mind how the problem gets solved, it has to weigh them, and may find a better fix than any of yours. -> Does an absent `draDrivers` need to mean "install everything"? It looks like -> compose-inference-cluster always sets the list, so this path only serves -> ServingStacks created before the field existed, and Modelplane has none to -> migrate. What if we required the field, or defaulted it to `[]`? That would -> remove the `None` branch in `join`, the `ExistingStandardUnfiltered` case and -> the comments in both functions that explain it. - -> Won't returning here delete a running cluster? I think Crossplane garbage -> collects composed resources that drop out of the desired state. Could the -> gate keep composing what already exists? - -> Is the list below still right for AMD pools? It says an existing cluster -> provides its own exporter and needs NVIDIA's dcgm-exporter chart, but this -> paragraph says the serving stack installs the AMD exporter. Should it say -> that only applies to NVIDIA pools? +These are real comments that follow the pattern. Phrase your own for the PR in +front of you rather than reusing their wording. + +> Won't `ready != len(refs)` always be true if `renderFail` is true? Or are +> there cases where we need both? + +> Does using kwargs here weaken the type checking we'd get from using +> `ModelReplica`? Could we instead form the ModelReplica model outside of +> `resource.update` and set `replica.spec.modelCacheRef` only if the +> ModelDeployment has one? + +> Do we need to hold composition until the gateway is Ready? With the anchor +> in place, I'd expect a route created during a rollout to be harmless. If so, +> always composing and gating only `RoutingReady` would let us drop this +> special case. + +> Won't this leave a stored prefill/decode replica's `ModelAccepted` and +> `ModelReady` as `Unknown`? On a fatal result Crossplane keeps the conditions +> a function sets, and marks every custom condition it didn't set as +> `Unknown` +> ([reconciler.go](https://github.com/crossplane/crossplane/blob/v2.4.0/internal/controller/apiextensions/composite/reconciler.go#L792-L803)). +> So a pair that's still serving would show `ModelReady=Unknown`. Phrase a correctness finding in the review summary this way too. @@ -258,15 +265,12 @@ The python-code-factoring and engineering-principles skills hold the rest. ## The Review Summary The summary opens with one hedged sentence on how close the PR looks to -mergeable, without repeating any inline comment: - -- "This looks ready to me." -- "This looks close. I have a few questions." -- "I think a couple of things need another look before this merges." -- "I think we should step back and talk about the approach." - -The design reviewer's verdict weighs most in choosing it. Don't grade the PR -beyond that: leave out verdicts like "well tested" or "reads well". +mergeable, without repeating any inline comment. It places the PR on a scale +from ready, through close with a few questions and needing another look at +specific things, to needing a step back on the approach. Say it in your own +words for this PR, naming what stands between it and merging where that's one +thing. The design reviewer's level weighs most in choosing it. Don't grade the +PR beyond that: leave out verdicts like "well tested" or "reads well". The rest of the summary explains that verdict, in a short paragraph at most, and the whole summary stays under 1,000 characters. Findings go inline, each diff --git a/.opencode/agents/review-correctness.md b/.opencode/agents/review-correctness.md index e6e486a92..7c5112d7d 100644 --- a/.opencode/agents/review-correctness.md +++ b/.opencode/agents/review-correctness.md @@ -24,13 +24,15 @@ can run any tool you need with `nix run`. The nix-tools and nix-flake skills cover how. A finding about external behaviour cites the upstream source or docs that show it. -The bar is a concrete input or sequence that triggers the bug, in a deployment -someone would run. Drop hypotheticals, states the schema rules out, and -backward compatibility concerns. Modelplane is pre-1.0. Don't review prose or +A strong finding gives a concrete input or sequence that triggers the bug, in +a deployment someone would run. Skip states the schema rules out, and backward +compatibility concerns, since Modelplane is pre-1.0. Don't review prose or style. Other reviewers cover them. Report each finding with its file, its line in the PR's head commit, a severity -(blocking, issue or nit), the failure, and the evidence: the input that -triggers it and the code path or test output that shows it. Leave out weak -findings, even if that leaves none. Don't edit any files in the repository. Do -scratch work under `/tmp`. +(blocking, issue or nit), the failure, and the evidence: the input that triggers +it and the code path or test output that shows it. Report every finding, +including ones you're unsure of, and give each a confidence: high, medium or +low. The review agent chooses what to post. Rather than holding back a finding +that seems minor or uncertain, say so in its confidence. Don't edit any files in +the repository. Do scratch work under `/tmp`. diff --git a/.opencode/agents/review-craft.md b/.opencode/agents/review-craft.md index 44efa4dc1..98de28afd 100644 --- a/.opencode/agents/review-craft.md +++ b/.opencode/agents/review-craft.md @@ -31,4 +31,7 @@ cover them. Report each finding with its file, its line in the PR's head commit, a severity (blocking, issue or nit), what's wrong, and the convention or code it departs -from. Leave out weak findings, even if that leaves none. Don't edit any files. +from. Report every finding, including ones you're unsure of, and give each a +confidence: high, medium or low. The review agent chooses what to post. Rather +than holding back a finding that seems minor or uncertain, say so in its +confidence. Don't edit any files. diff --git a/.opencode/agents/review-design.md b/.opencode/agents/review-design.md index 51c30e7d3..027cddb2c 100644 --- a/.opencode/agents/review-design.md +++ b/.opencode/agents/review-design.md @@ -34,18 +34,19 @@ For the code, ask: Don't review code structure, prose or correctness bugs. Other reviewers cover them. -Start your report with how far the PR's design looks from mergeable, in one of -these, or a sentence close to one: +Start your report with one of these levels, for how far the PR's design looks +from mergeable: -- The design looks sound. -- The design looks sound, with a few questions. -- I think some design questions need settling before this merges. -- I think we should step back and talk about the approach. +- **Sound:** nothing to raise about the design. +- **Questions:** the design looks right, with questions that probably won't + change it. +- **Rework:** some design questions need answers before this merges. +- **Rethink:** the approach itself needs discussing. Then report each design question with the file and line it concerns. Almost every design question is about a specific field, section of a design doc or line of code. Leave out the line only for a question about the PR as a whole, such as -whether to split it. Give the question, and your evidence: the committed design, -the issue or the code you're comparing against. Report only the questions a -maintainer would want asked, usually no more than three. If the design looks -sound, say so and report nothing else. Don't edit any files. +whether to split it. Give the question, your evidence (the committed design, the +issue or the code you're comparing against), and a confidence: high, medium or +low. Report every question you'd consider asking. You report to the review +agent, which chooses which to post. Don't edit any files. diff --git a/.opencode/agents/review-prose.md b/.opencode/agents/review-prose.md index c54d840ce..0633870fc 100644 --- a/.opencode/agents/review-prose.md +++ b/.opencode/agents/review-prose.md @@ -37,5 +37,7 @@ Don't review code structure or correctness. Other reviewers cover them. Report each finding with its file, its line in the PR's head commit, a severity (blocking, issue or nit), what's wrong, and the evidence: the quoted text, and -the code that contradicts it where there is some. Leave out weak findings, even -if that leaves none. Don't edit any files. +the code that contradicts it where there is some. Report every finding, +including ones you're unsure of, and give each a confidence: high, medium or +low. The review agent chooses what to post. Rather than holding back a finding +that seems minor or uncertain, say so in its confidence. Don't edit any files. diff --git a/.opencode/agents/review.md b/.opencode/agents/review.md index 94c0ad3ae..20c33014f 100644 --- a/.opencode/agents/review.md +++ b/.opencode/agents/review.md @@ -94,14 +94,15 @@ not work to forward: get fifteen. The workflow refuses a review with more than 25 comments. When a large PR has more to say than is useful in one review, keep the findings that matter most and drop nits first. -- Keep every point the design reviewer makes. Don't merge one question into - another or drop its evidence, above all where it cites a principle from a - committed design. Each still goes where any finding goes: inline on the line +- Post every design question the design reviewer rates high or medium + confidence, unless it fails the bar. Don't merge one question into another + or drop its evidence, above all where it cites a principle from a committed + design. Each still goes where any finding goes: inline on the line it concerns, and in the summary only if no line fits. The length rules for comments apply to design questions too. -- If the design reviewer thinks the design needs settling, or the approach - rethinking, hold back the other reviewers' findings on naming, tests and - style. They're wasted if the design changes. Keep correctness findings that +- If the design reviewer rates the design Rework or Rethink, hold back the + other reviewers' findings on naming, tests and style. They're wasted if the + design changes. Keep correctness findings that would matter whatever the design becomes. ## 4. Write the review @@ -112,11 +113,11 @@ bot. The skill's rules about writing under a person's name don't apply. Write the summary and each comment as the reviewing-pull-requests skill describes. Its section on phrasing matters most: ask, explain and suggest for anything about correctness or design, even when the reviewers stated it as fact, and be direct -only about style. The summary's opening sentence matches the design reviewer's -verdict, and mustn't soften it. Attach every comment to a line the PR changed. -That's the line a finding is about, or for code the PR didn't change, the change -that causes the problem. Keep the summary to the verdict and why, under 1,000 -characters, as the skill describes. +only about style. The summary's opening sentence reflects the design reviewer's +level, in your own words, and mustn't soften it. Attach every comment to a line +the PR changed. That's the line a finding is about, or for code the PR didn't +change, the change that causes the problem. Keep the summary to the verdict and +why, under 1,000 characters, as the skill describes. Write the review as markdown files in the directory you're given. A linter checks each one as you write it. Don't hard-wrap them. GitHub shows a newline in From 0c4b207a9909f4a95e7cfc0b3fc57e90f5c08646 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Mon, 2 Nov 2026 01:22:52 -0800 Subject: [PATCH 09/13] Give each review rule one home The review skills repeated each other and in places disagreed. Every reviewer loaded reviewing-pull-requests and its checklist of what to look for, though each reviewer checks one category, and writing-style mixed general prose rules with design doc rules that the review agent had to override. This commit keeps one copy of each rule, where the agents that need it load it. Each reviewer's checklist is in its agent, the Crossplane failure modes are in crossplane-python-functions, and the design doc rules are in design-docs. The pre-1.0 rule is in AGENTS.md, since every agent reads it. reviewing-pull-requests keeps what the review agent needs to post: the bar, phrasing, where comments go and the summary. Signed-off-by: Nic Cope --- .../crossplane-python-functions/SKILL.md | 16 ++ .agents/skills/design-docs/SKILL.md | 36 +++++ .../references/voice.md | 0 .agents/skills/reviewing-designs/SKILL.md | 15 +- .../skills/reviewing-pull-requests/SKILL.md | 153 +++--------------- .agents/skills/writing-style/SKILL.md | 106 +++--------- .../references/de-slop-review.md | 19 +-- .opencode/agents/review-correctness.md | 12 +- .opencode/agents/review-craft.md | 38 +++-- .opencode/agents/review-design.md | 4 +- .opencode/agents/review-prose.md | 17 +- .opencode/agents/review.md | 27 ++-- AGENTS.md | 4 + 13 files changed, 159 insertions(+), 288 deletions(-) rename .agents/skills/{writing-style => design-docs}/references/voice.md (100%) diff --git a/.agents/skills/crossplane-python-functions/SKILL.md b/.agents/skills/crossplane-python-functions/SKILL.md index 0fe6e7dba..b4a3f45f6 100644 --- a/.agents/skills/crossplane-python-functions/SKILL.md +++ b/.agents/skills/crossplane-python-functions/SKILL.md @@ -34,6 +34,22 @@ from models.io.crossplane.m.helm.release import v1beta1 as helmv1beta1 The rest of this skill covers how Crossplane behaves, which is what most bugs in a function get wrong. +## Gotchas + +These come up repeatedly in review, and are easy to miss: + +- **Dropping desired state deletes things.** A function that returns early, or + skips composing a resource it composed before, causes Crossplane to garbage + collect that resource. A gate on a dependency must keep composing a resource + once it exists. See Gating Resources on Dependencies below. +- **Composed resource names.** Use the SDK's naming helpers, and check names + stay within Kubernetes length limits and can't collide across replicas or + clusters. +- **Readiness** reflects the condition the real resource sets, never a + hardcoded value. +- **Selectors inside lists** need the referenced resource's id instead. See the + composition function section of CONTRIBUTING.md. + ## Composing Resources Use Pydantic models for everything with a generated model. They catch field diff --git a/.agents/skills/design-docs/SKILL.md b/.agents/skills/design-docs/SKILL.md index dd60e618a..bb08d9fb1 100644 --- a/.agents/skills/design-docs/SKILL.md +++ b/.agents/skills/design-docs/SKILL.md @@ -137,6 +137,42 @@ reconstruct, then again for what remains. Trade paragraphs of prose for a worked example wherever one carries the point. Run vale over the whole doc and a fresh-context de-slop review, and fix both until they come back clean. +For voice, the section-by-section skeleton in more depth, and worked examples of +the target, read [references/voice.md](references/voice.md) when drafting from +scratch. + +### Writing the End State + +A design records direction. It must not actively mislead, though it may show +things not yet built (standard 4). + +- Don't link issues or PRs in the same repo. They expose the doc's real + chronology, and a design should read as the first artifact in the repo. Link + generously outward to upstream issues, specs, talks and prior art. +- Don't link a durable doc to a scratch file you'll delete. +- A superseded doc isn't rewritten. It gets a one or two sentence preface + saying it remains for historical context. At some point a design stops + absorbing revisions and gets extended by supplementary designs. +- Goals appear positively, as what gets delivered, and at project level rather + than scoped to a release. +- Leave out the limitations currently blocking the end state. Where the + mechanism isn't decided, stay deliberately vague and state the requirement. +- Opinions belong inline beside the thing they concern. + +### Examples + +Beyond standard 9: + +- **Calibrate realistically.** Selectors ask for what a model actually needs, + and examples reflect the scale users really run. +- **Cut examples that don't do any work.** One that only shows an off-the-shelf + resource doesn't add anything. Merge a pair that make the same point. +- **Annotate lightly.** Brief YAML comments where semantics aren't obvious, and + `# Omitted for brevity` where you trim. +- **Cover the awkward case.** Worked examples are how a proposal proves it + handles the topologies it claims to, so include the pathological one. +- **Collapse long artifacts,** so they illustrate without bloating the doc. + ## Refactoring Refactoring a poor doc, often an AI-written one, is surgery on the document's diff --git a/.agents/skills/writing-style/references/voice.md b/.agents/skills/design-docs/references/voice.md similarity index 100% rename from .agents/skills/writing-style/references/voice.md rename to .agents/skills/design-docs/references/voice.md diff --git a/.agents/skills/reviewing-designs/SKILL.md b/.agents/skills/reviewing-designs/SKILL.md index 27c527306..5c8427e74 100644 --- a/.agents/skills/reviewing-designs/SKILL.md +++ b/.agents/skills/reviewing-designs/SKILL.md @@ -157,18 +157,9 @@ someone builds the deferred part properly. ## How to Raise It -Phrase design questions as reviewing-pull-requests asks for any correctness -question: ask, explain, suggest. A design question especially wants a hedge, -because you're missing context the author has. - -- State your assumption and invite correction: "I assume the main cost here is - X, right?" -- Sketch an alternative in YAML where it helps, and say you haven't thought it - through: "please poke holes in this rather than taking it as-is". -- Give the cost of your suggestion, not only its benefit. -- Separate a gap in the design from a gap in how the doc explains it. -- Offer more than one option where they come naturally, and say when you don't - mind which. +Phrase design questions as reviewing-pull-requests describes: ask, explain, +suggest. Where it helps, sketch an alternative in YAML and say you haven't +thought it through: "please poke holes in this rather than taking it as-is". Getting a fact wrong costs the whole review its credibility. Check a claim about how Crossplane, Kubernetes or an upstream project behaves against its diff --git a/.agents/skills/reviewing-pull-requests/SKILL.md b/.agents/skills/reviewing-pull-requests/SKILL.md index cead7b9b6..f1fed2e8e 100644 --- a/.agents/skills/reviewing-pull-requests/SKILL.md +++ b/.agents/skills/reviewing-pull-requests/SKILL.md @@ -44,9 +44,9 @@ Don't raise these at all: XRD schema or a CEL rule enforces puts one invariant in two places. - **A second enforcement point.** If an invariant could be a CEL rule, ask for the CEL rule rather than a check in the function. -- **Alternative architectures or new features.** Design and API direction - belong to the maintainers. Where a change has real design weight, ask about - it once in the summary. Don't spread it across inline comments. +- **Requests to do more.** Don't ask a PR to add features or take a different + architecture than it sets out to. Design questions about what it does are + the design reviewer's, and the reviewing-designs skill covers them. - **Settled questions.** Read the existing review threads first. Don't re-raise something already discussed, resolved, or declined. - **Missing E2E coverage.** E2E runs in a separate, label-gated workflow. @@ -103,7 +103,10 @@ front of you rather than reusing their wording. > ([reconciler.go](https://github.com/crossplane/crossplane/blob/v2.4.0/internal/controller/apiextensions/composite/reconciler.go#L792-L803)). > So a pair that's still serving would show `ModelReady=Unknown`. -Phrase a correctness finding in the review summary this way too. +A design question especially needs the hedge, because the author has context +you don't. State your assumption and invite correction ("I assume the main cost +here is X, right?"). Give the cost of your suggestion, not only its benefit, and +say whether the gap is in the design or only in how the doc explains it. **Be direct about style, readability and maintainability.** "Please use the established pattern here, because..." is fine, as long as the comment gives the @@ -119,8 +122,7 @@ fixed line. Mark minor points with `Nit:`. Write short comments. Most run two to four sentences, ending with the suggestion. -Make one point per comment, attached to the line it's about, or for code the -PR didn't change, to the change that causes the problem. If the same issue +Make one point per comment. If the same issue recurs, comment once and add "(here and elsewhere)" rather than repeating the comment. @@ -141,126 +143,15 @@ Don't use GitHub suggestion blocks. Applying one creates a commit without a Write plainly, as a maintainer would. The writing-style skill applies to review comments as much as to docs. -## What to Look For +## Where Comments Go -These are in rough order of how often maintainers raise them, which isn't the -same as how much each matters. +Attach every comment to a line the PR changed, in the most relevant place: -### Comments and docs that don't match the code - -This is the most common category, and authors accept it nearly every time. -Look for: - -- Comments, docstrings, field descriptions and docs that describe old - behaviour, or that an agent copied from elsewhere and that still name the - wrong type, cloud or verb. -- Claims that aren't true. A label nothing reads, the wrong component - reading a field, or a stale pinned version. -- Invented rationale: a plausible reason for a decision that nothing in the - code, the issue or the design supports. -- Overclaiming. Docs describe what the project does today, not what it aims to - do. -- Comments that narrate history, such as "we used to", "this replaces", "now" - or replies to an earlier review round. History belongs in the commit - message. -- Commentary that costs the reader more than it gives them: comments three - times as long as they need to be, or that restate the code. - -### Missing "why" comments and contracts - -Where a reader would have to dig to understand something, ask for a comment -that explains why. Examples are a surprising choice, a foot-gun, a magic -number, an ignored error, or an external constraint the code works around. - -### Speculative complexity - -Push back on fields, options, knobs, helpers, abstractions and shared modules -added before anything needs them. Ask "does this need to exist?" about each -new one. Also ask whether the PR needs: - -- Guards for states that can't happen. -- Fallbacks that can't matter in practice. -- Status fields that duplicate what other state already implies. -- A special case where one code path would serve everyone. -- A helper called once, or one that does almost nothing. -- Tests that can't fail, that duplicate another case, or that only detect - change. - -### Consistency with what's already there - -New code follows the established pattern for what it's doing: test style, -naming, label keys, how a function waits on a dependency, how each cloud's -composition does the same job. When a PR adds a helper, check whether the SDK, -a dependency or the repo already has one. When a PR fixes something in one -cloud's function, check the others. - -An agent may have written an existing pattern with nobody reviewing it -closely. Say when a finding -rests on precedent alone, and don't hold a PR to a pattern that's plainly -wrong. The fix for a wrong pattern is an issue to change it everywhere. - -### Names - -A name says exactly what the thing is or does. Look for a name that drifted -when the behaviour changed, a loaded term reused for something else (Ready, -Synced, "managed"), a name that doesn't match its siblings, and grab-bag module -names like `util` or `common`. - -### Crossplane correctness - -These come up repeatedly and are easy to miss: - -- **Dropping desired state deletes things.** A composition function that - returns early, or skips composing a resource it composed before, causes - Crossplane to garbage collect that resource. A gate on a dependency must - keep composing a resource once it exists. -- **Composed resource names.** Use the SDK's naming helpers, and check names - stay within Kubernetes length limits and can't collide across replicas or - clusters. -- **Readiness.** Readiness reflects the condition the real resource sets, - never a hardcoded value. -- **Selectors inside lists** need the referenced resource's id instead. See - the composition function section of CONTRIBUTING.md. - -The crossplane-python-functions skill covers the rest of the function -contract. - -### API design - -The maintainers own API design, so raise these as questions rather than -verdicts. For XRDs, follow the krm-api-design skill: - -- Question bool fields that might need a third state. -- Mutually exclusive options want a discriminator field. -- Spec holds desired state and status holds observed state. -- Each field description reads well in `kubectl explain`. -- Enforce constraints in the schema, with CEL, rather than in code. - -### Tests - -New behaviour gets a new case in the existing table, following the Tests -section of CONTRIBUTING.md. Ask for a test where new behaviour or an error -path has none. Question a new test where the cases above it already cover -the behaviour. - -### Scope - -Unrelated changes, such as dependency bumps, drive-by refactors, new test -frameworks or new tools, belong in their own PR, and a new tool or framework -needs a decision first. - -## Language Notes - -Python, which is nearly all of Modelplane: - -- Deserialize observed resources into the generated Pydantic models rather - than chaining `.get()` calls on dicts. -- Import modules, not names from inside them. -- Fix types rather than adding `ty: ignore`. A `cast`, a `Literal` alias or - `Final` constants usually let the checker prove the value. -- Never hand-edit generated code under `schemas/python/`. - -The python-code-factoring and engineering-principles skills hold the rest. +- A finding about a line the PR changed goes on that line. +- A finding about code or docs the PR didn't change goes on the change that + causes it. If a comment or docs page elsewhere is now untrue, comment on the + change that made it untrue, and say which comment or link the page. +- A design question goes on the field, design doc section or code it's about. ## The Review Summary @@ -272,16 +163,10 @@ words for this PR, naming what stands between it and merging where that's one thing. The design reviewer's level weighs most in choosing it. Don't grade the PR beyond that: leave out verdicts like "well tested" or "reads well". -The rest of the summary explains that verdict, in a short paragraph at most, -and the whole summary stays under 1,000 characters. Findings go inline, each -in the most relevant place: - -- A finding about code the PR didn't change goes on the change that causes it. - If a comment elsewhere is now stale, comment on the change that made it - stale, and say which comment. -- For docs the PR doesn't touch, comment on the change that makes them wrong, - and link the page. -- A design question goes on the field, design doc section or code it's about. +The rest of the summary explains that verdict in two or three sentences, and +the whole summary stays under 1,000 characters. It says what the verdict rests +on without restating the inline comments about it, which the author reads +next. Every finding goes inline, as the previous section describes. A finding about the PR as a whole, such as whether to split it, belongs in the summary when it's the reason for the verdict. Findings about the commit messages diff --git a/.agents/skills/writing-style/SKILL.md b/.agents/skills/writing-style/SKILL.md index d62a3ef2f..e31fa5a53 100644 --- a/.agents/skills/writing-style/SKILL.md +++ b/.agents/skills/writing-style/SKILL.md @@ -10,14 +10,11 @@ maintainers read every word and delete whatever the reader doesn't need, so aim for a draft they'd leave as it is. The quotes below are Nic's corrections to agents, kept as evidence for each rule. -Most of this skill covers what goes wrong. An LLM's instinct for a good design -doc is roughly right about voice and section structure, and badly wrong about -length, honesty about rationale, and who the document addresses. So the failure -modes come first. +Most of this skill covers what goes wrong. An LLM's instinct for prose is +roughly right about voice, and badly wrong about length, honesty about +rationale, and who the document addresses. So the failure modes come first. + -For voice, the section-by-section skeleton, and worked examples of the target, -read [references/voice.md](references/voice.md). Read it when drafting from -scratch. Skip it when tightening prose that already exists. ## When to Use This (vs Other Skills) @@ -27,7 +24,8 @@ comments. CONTRIBUTING.md sets the conventions specific to commits, PRs and issues, and "Cut Hard, Twice", "Never Invent Rationale", and "Substantiate or Delete" below apply to them unchanged. -Use **design-docs** for the structure of a design doc. +Use **design-docs** for everything specific to a design doc, including its +structure, examples and voice. Use **reviewing-pull-requests** for what to raise in a review, and this skill for how to word it. @@ -46,7 +44,7 @@ phrasing in one comment. ## Before You Draft -Establish three things, and ask rather than guess: +Establish three things. Ask rather than guess when someone's there to answer: 1. **Audience.** Who reads this? Maintainers, contributors, users, a partner's engineers? "High level but deeply technical" produces a different document @@ -153,12 +151,8 @@ rename without saying why. A plausible reason invented to fill the gap is worse than no reason, because the reader can't tell it apart from a real one and it becomes part of the permanent record. -If you don't know why, ask. Nic has said so outright: - -> DO NOT invent justification unless you know for sure why something is like it -> is. You can ask me if you don't know. - -When he catches it the response is blunt: "You just made that all up." +If you don't know why, ask, or leave the reason out. When Nic catches an +invented reason, the response is blunt: "You just made that all up." This extends past rationale: @@ -212,64 +206,13 @@ section takes up more space than any other in the doc. Should it?" - One concept per list entry. Don't bundle rate limiting and authentication under "identity", and don't drop an item while reorganising. -### Write the Design as If It Preceded the Code - -A design records direction. It concentrates on APIs and UX with broad-strokes -mechanics. It doesn't mirror the code, and it isn't a recipe to build it. It -must not actively mislead, though it may show things not yet built. - -So write in the present tense, describing the current position as though it were -the first and only one: - -- No changelog framing, since the reader doesn't care how a previous - unreleased iteration worked. -- Discarded thinking belongs in Alternatives Considered, gathered there rather - than scattered inline. -- No links to issues or PRs in the same repo. They expose the document's real - chronology, and a design should read as the first artifact in the repo. Link - generously outward to upstream issues, specs, talks, and prior art. -- A superseded document isn't rewritten. It gets a one or two sentence preface - saying it remains for historical context. -- Don't link a durable document to a scratch file you'll delete. - -At some point a design stops absorbing revisions and gets extended by -supplementary designs. Prefer minimal surgery to a rewrite, because it's cheaper -to review. - -A proposal proposes the intended end state, and leaves out the limitations -currently blocking it. Goals appear positively, as what gets delivered, and at -project level rather than scoped to a release. Don't confess open unknowns in -passing. Stay deliberately vague where the mechanism remains unsettled, and -state the requirement instead. - -Leave out an Open Questions section. A design takes a position. Opinions belong -inline beside the thing they concern, rather than in a trailing "positions" or -"recommendations" section. - ### Examples Are Real An example does more work than the prose around it, so give it the same -scrutiny. - -- **Reuse validated artifacts from the repo.** Copy the tested manifest instead - of composing one from memory, and use the same example throughout the - document, including the executive summary. -- **Show whole objects.** A full ModelDeployment, and not a `spec:` fragment. -- **If you can't verify it, say so.** "I couldn't build a credible example here" - is better than a plausible fabrication. -- **Calibrate realistically.** Selectors should ask for what a model actually - needs, and examples should reflect the scale users really run. -- **Cut examples that don't do any work.** One that only shows an off-the-shelf - resource doesn't add anything a reader lacks. Merge a pair that make the same - point. A contrived example damages credibility more than an absent one, so - omit rather than force. -- **Annotate lightly.** Brief YAML comments where semantics aren't obvious, and - `# Omitted for brevity` where you trim. -- **Cover the awkward case.** Worked examples are how a proposal proves it - handles the topologies it claims to, so include the pathological one. - -Long verified artifacts belong in collapsed blocks, so they illustrate without -bloating the document. +scrutiny. Copy a tested artifact from the repo rather than composing one from +memory, and show whole objects. If you can't build a credible example, say so, +because a contrived one damages credibility more than an absent one. The +design-docs skill covers examples in designs in more depth. ## Naming @@ -401,9 +344,9 @@ over. whether a section argues with a reviewer or invents a reason, or whether it gives four paragraphs to something that deserves one. Dispatch a subagent briefed with [references/de-slop-review.md](references/de-slop-review.md), plus -the audience, the purpose, and one or two existing designs under `design/` for -the project's style. Fix what it finds and repeat until a reviewer comes back -clean, because one pass rarely suffices. +the audience, the purpose, and one or two existing documents of the same kind +for the project's style. Fix what it finds and repeat until a reviewer comes +back clean, because one pass rarely suffices. Use a subagent rather than reviewing inline. Fresh context matters because the drafting agent can't see its own reactive framing. @@ -414,8 +357,8 @@ are orthogonal. ## Do Not -- Don't add AI attribution. Text published under a person's name reads as - theirs, so write first person or drop the pronoun, and never refer to the +- Don't add AI attribution. Text under a person's name reads as theirs. + Write it in the first person or drop the pronoun, and never refer to the author in the third person. - Don't lose a person's edits. Where someone copy edits alongside you, their edits win, and any later rewrite keeps them. Re-read the file before @@ -426,13 +369,12 @@ are orthogonal. 1. Cut 30%, then cut another 10-20%, before anyone reads it. 2. Write for the reader, never against the last draft or the last comment. -3. Never invent rationale. Ask instead. +3. Never invent rationale. Ask, or leave it out. 4. Support every claim with a mechanism and a baseline, or delete it. 5. Space signals importance, so keep sections and list entries proportional. -6. A design reads as though it preceded the code. -7. Examples are real, whole, verified, and doing work. -8. Name things deliberately, with established terms that don't mislead in the +6. Examples are real, whole, verified, and doing work. +7. Name things deliberately, with established terms that don't mislead in the edge case, and one term per concept. -9. Reasoning belongs in prose. Structure is for structured content. -10. Run Vale, then a fresh-context subagent, and loop until both come back - clean. +8. Reasoning belongs in prose. Structure is for structured content. +9. Run Vale, then a fresh-context subagent, and loop until both come back + clean. diff --git a/.agents/skills/writing-style/references/de-slop-review.md b/.agents/skills/writing-style/references/de-slop-review.md index 03063188a..067225e7b 100644 --- a/.agents/skills/writing-style/references/de-slop-review.md +++ b/.agents/skills/writing-style/references/de-slop-review.md @@ -15,7 +15,8 @@ Tell the reviewer: 1. **Audience.** Who reads this document. 2. **Purpose.** What decision it serves. -3. **Project style.** Point at one or two existing designs under `design/`, so +3. **Project style.** Point at one or two existing documents of the same kind, + so the reviewer can compare rather than guess. Then give it the instructions below. @@ -92,19 +93,9 @@ Read the document end to end as somebody meeting it for the first time: ### 6. Structure -Check the document against the design-docs skill's skeleton: executive summary, -background, goals, proposal, alternatives considered, with future improvements -where they apply. Report: - -- An Open Questions or Risks section, when a design should take a position. -- A trailing positions, recommendations, or summary section. Opinions belong - beside the thing they concern. -- Alternatives scattered inline rather than gathered in their own section. -- Headings, bold, or bullets used for decoration. Reasoning belongs in prose. -- Links to issues or PRs in the same repo, which expose the document's - chronology. -- Goals scoped to a release rather than to the project, or framed as the absence - of a problem rather than as what gets delivered. +If the document is a design doc, check it against the design-docs skill's +standard, and report each point it misses. For any document, report headings, +bold or bullets used for decoration, since reasoning belongs in prose. ### 7. Length diff --git a/.opencode/agents/review-correctness.md b/.opencode/agents/review-correctness.md index 7c5112d7d..3ee0d68dc 100644 --- a/.opencode/agents/review-correctness.md +++ b/.opencode/agents/review-correctness.md @@ -6,12 +6,11 @@ mode: subagent --- You review whether a Modelplane pull request works. Load the -reviewing-pull-requests, crossplane-python-functions and engineering-principles -skills. +crossplane-python-functions and engineering-principles skills. Assume the author is overconfident, and look for what's wrong. That includes logic errors, inputs the schema allows that the code mishandles, and misuse of -the Crossplane function SDK or Kubernetes APIs. The reviewing-pull-requests +the Crossplane function SDK or Kubernetes APIs. The crossplane-python-functions skill lists Crossplane failure modes that recur. Check whether a bug the PR fixes also exists in the sibling code path or another cloud's function. @@ -24,10 +23,9 @@ can run any tool you need with `nix run`. The nix-tools and nix-flake skills cover how. A finding about external behaviour cites the upstream source or docs that show it. -A strong finding gives a concrete input or sequence that triggers the bug, in -a deployment someone would run. Skip states the schema rules out, and backward -compatibility concerns, since Modelplane is pre-1.0. Don't review prose or -style. Other reviewers cover them. +A strong finding gives a concrete input or sequence that triggers the bug, in a +deployment someone would run. Skip states the schema rules out. Don't review +prose or style. Other reviewers cover them. Report each finding with its file, its line in the PR's head commit, a severity (blocking, issue or nit), the failure, and the evidence: the input that triggers diff --git a/.opencode/agents/review-craft.md b/.opencode/agents/review-craft.md index 98de28afd..d31480265 100644 --- a/.opencode/agents/review-craft.md +++ b/.opencode/agents/review-craft.md @@ -6,28 +6,38 @@ mode: subagent --- You review the craft of a Modelplane pull request's code. Load the -reviewing-pull-requests, engineering-principles and python-code-factoring -skills, plus crossplane-python-functions for composition functions and -krm-api-design for XRDs. The Tests section of CONTRIBUTING.md governs function -tests. +engineering-principles and python-code-factoring skills, plus +crossplane-python-functions for composition functions and krm-api-design for +XRDs. The Tests section of CONTRIBUTING.md governs function tests. For every helper, field, option, guard, test and dependency the PR adds, ask whether it needs to exist. Look for: -- Speculative complexity, and guards for states the schema rules out. -- Code that solves a problem differently from how the repo already solves it, - including how the other clouds' functions do the same job. +- Speculative complexity: knobs, abstractions and shared modules nothing needs + yet, a helper called once, or a fallback that can't matter in practice. +- Guards for states the schema or a CEL rule already rules out, and status + fields that duplicate what other state implies. +- Code that solves a problem differently from how the repo already solves it. + That includes how the other clouds' functions do the same job, and helpers + the SDK, a dependency or the repo already has. +- A fix made in one cloud's function and missing from the others. - Code the change made redundant but left in place. -- Names that don't say what the thing is. -- Tests that can't fail, duplicate another case, or don't follow the table +- Names that don't say what the thing is, that drifted when the behaviour + changed, that don't match their siblings, or that reuse a loaded term such as + Ready, Synced or "managed". Grab-bag modules like `util` or `common`. +- New behaviour or an error path with no test case. Tests that can't fail, + duplicate another case, only detect change, or don't follow the table pattern. -- Python project style, such as using the generated Pydantic models rather - than dicts, and importing modules rather than names. +- Python project style, as python-code-factoring describes: the + generated Pydantic models replace dicts, and a type error gets fixed rather + than suppressed. +- Unrelated changes, such as a dependency bump, a drive-by refactor or a new + tool, that belong in their own PR. An existing pattern may itself be poor. Say when a finding rests on precedent -alone. Raise API design only as a question, and don't propose alternative -architectures. Don't review prose or hunt for runtime bugs. Other reviewers -cover them. +alone. Other reviewers cover API design, alternative architectures, prose and +runtime bugs. Skip anything CI enforces: ruff, ty, the function test-style +checker and the list-selector checker. Report each finding with its file, its line in the PR's head commit, a severity (blocking, issue or nit), what's wrong, and the convention or code it departs diff --git a/.opencode/agents/review-design.md b/.opencode/agents/review-design.md index 027cddb2c..b75e03ae8 100644 --- a/.opencode/agents/review-design.md +++ b/.opencode/agents/review-design.md @@ -6,8 +6,8 @@ mode: subagent --- You review the premise and design of a Modelplane pull request. Load the -reviewing-designs and reviewing-pull-requests skills, plus krm-api-design if -the PR changes an XRD or proposes an API. +reviewing-designs skill, plus krm-api-design if the PR changes an XRD or +proposes an API. Treat the design docs committed under `design/` on the base branch as settled. Read the ones the PR builds on. Then look at the PR two ways. diff --git a/.opencode/agents/review-prose.md b/.opencode/agents/review-prose.md index 0633870fc..331e83abc 100644 --- a/.opencode/agents/review-prose.md +++ b/.opencode/agents/review-prose.md @@ -6,21 +6,26 @@ mode: subagent --- You review the prose of a Modelplane pull request: whether it's true, and -whether it repays the time it takes to read. Load the reviewing-pull-requests -and writing-style skills, and the design-docs skill if the PR touches -`design/`. The writing-style skill's section on reviewing someone else's prose -is your brief, and its failure modes are your checklist. +whether it repays the time it takes to read. Load the writing-style skill, and +the design-docs skill if the PR touches `design/`. The writing-style skill's +section on reviewing someone else's prose is your brief, and its failure modes +are your checklist. Review every comment, docstring, XRD field description, doc page and design doc the PR adds or changes. Check each claim against the code, not against other prose. Look for: - Claims the code doesn't support, including stale ones the PR left behind - when it changed the code they describe. + when it changed the code they describe, and text an agent copied from + elsewhere that still gives the wrong type, cloud or verb. +- Overclaiming: docs that describe what the project aims to do rather than + what it does today. - Rationale nothing in the code, issue or design backs up. - Comments that narrate the change or its history. - Commentary much longer than what it says, or that restates the code. -- Places a reader needs a "why" and doesn't get one. +- Places a reader needs a "why" and doesn't get one, such as a surprising + choice, a foot-gun, a magic number, an ignored error, or an external + constraint the code works around. - Docs examples that don't match the real API. - Design docs that miss the design-docs standard. diff --git a/.opencode/agents/review.md b/.opencode/agents/review.md index 20c33014f..283a196b7 100644 --- a/.opencode/agents/review.md +++ b/.opencode/agents/review.md @@ -108,16 +108,11 @@ not work to forward: ## 4. Write the review Load the writing-style skill before you write, and follow it. Don't stop to ask -questions, since nobody is there to answer. You post as the project's review -bot. The skill's rules about writing under a person's name don't apply. Write -the summary and each comment as the reviewing-pull-requests skill describes. Its -section on phrasing matters most: ask, explain and suggest for anything about -correctness or design, even when the reviewers stated it as fact, and be direct -only about style. The summary's opening sentence reflects the design reviewer's -level, in your own words, and mustn't soften it. Attach every comment to a line -the PR changed. That's the line a finding is about, or for code the PR didn't -change, the change that causes the problem. Keep the summary to the verdict and -why, under 1,000 characters, as the skill describes. +questions, since nobody is there to answer. Write the summary and each comment +as the reviewing-pull-requests skill describes. Phrase anything about +correctness or design as a question, even when the reviewers stated it as fact. +The summary's opening sentence reflects the design reviewer's level, in your +own words, and mustn't soften it. Write the review as markdown files in the directory you're given. A linter checks each one as you write it. Don't hard-wrap them. GitHub shows a newline in @@ -128,13 +123,12 @@ its line in the PR's head commit: ```markdown --- -path: functions/compose-model-cache/function/fn.py +path: functions/compose-inference-cluster/function/fn.py line: 42 --- -Won't returning here delete the cache's PVC? It looks like Crossplane garbage -collects composed resources that drop out of the desired state. Could the gate -keep composing the PVC once it exists? +Nit: `pool_names` reads as a list of names, but it holds `NodePool` objects. +`pools` would match its siblings. ``` Fix every error the linter reports on these files. When you've written them @@ -147,9 +141,8 @@ Vale catches phrasing, not padding, invented reasons or a comment that argues past its point. Dispatch a subagent with fresh context to catch those. Give it the writing-style skill's `references/de-slop-review.md` and the review directory. Tell it the audience is the PR's author and the maintainers, the -purpose is to tell them what to change and why, and the reviewing-pull-requests -skill's phrasing rules replace the brief's design-doc examples. It skips the -brief's structure check, which is for design docs. +purpose is to tell them what to change and why, and that each comment phrases +correctness and design findings as questions on purpose. Fix what it reports in the wording. Don't add, drop or change findings because of it, since it reviews prose, not substance. Then run Vale over the directory diff --git a/AGENTS.md b/AGENTS.md index d11a50c60..c05e0a5c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,3 +4,7 @@ Read [`CONTRIBUTING.md`](CONTRIBUTING.md) before making any change, and follow it. It is the contribution process: development setup, checks, commit and pull request conventions, and writing style. Contributions that don't follow it will be closed. + +Modelplane is pre-1.0, and nothing running it needs migrating. Don't add or ask +for backward compatibility, migration paths or deprecations. A breaking change +that makes an API better is welcome. From b620f4f0f333da0fabc4d66aee6e1b57fa418594 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Mon, 2 Nov 2026 11:31:06 -0800 Subject: [PATCH 10/13] Post long review comments rather than dropping them The workflow dropped any comment with more than 600 characters of prose, and a smoke test lost half a review that way, including its most important finding. Now the workflow posts a long comment and logs it, and the review agent measures its comments and summary before it finishes, and trims them by cutting words rather than findings. Signed-off-by: Nic Cope --- .agents/skills/reviewing-pull-requests/SKILL.md | 6 +++--- .opencode/agents/review.md | 11 +++++++++++ hack/post_review.py | 11 ++++++----- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/.agents/skills/reviewing-pull-requests/SKILL.md b/.agents/skills/reviewing-pull-requests/SKILL.md index f1fed2e8e..7c8980d88 100644 --- a/.agents/skills/reviewing-pull-requests/SKILL.md +++ b/.agents/skills/reviewing-pull-requests/SKILL.md @@ -129,9 +129,9 @@ comment. A comment that proposes an alternative may also sketch it, as a short YAML or code block with a sentence or two on what it buys. Show only the parts that differ, not a full implementation. Proposing an alternative is the only -reason a comment runs longer. Importance isn't one. The prose of a comment, -outside any code block, has a hard limit of 600 characters. The workflow drops -a comment that goes over. +reason a comment runs longer. Importance isn't one. Keep a comment's prose, +outside any code block, under about 600 characters. Get there by cutting +words, never by dropping a finding. Don't prescribe a fix in detail. Sketch it, or link to an example, and let the author work it out. Authors feed comments to their own agents, and a diff --git a/.opencode/agents/review.md b/.opencode/agents/review.md index 283a196b7..f798fbb8b 100644 --- a/.opencode/agents/review.md +++ b/.opencode/agents/review.md @@ -135,6 +135,17 @@ Fix every error the linter reports on these files. When you've written them all, run `vale` over the directory from the repo root, after `vale sync` if its styles are missing, and fix what it reports too. +Then measure each file's prose, leaving out front matter and code blocks: + +```bash +cd _output/review && for f in summary.md comments/*.md; do + echo "$(awk 'NR==1 && /^---$/ {fm=1; next} fm && /^---$/ {fm=0; next} fm {next} /^```/ {cb=!cb; next} !cb' "$f" | tr -s ' \n' ' ' | wc -c) $f" +done +``` + +Trim the summary to under 1,000 characters and each comment to under about 600 +by cutting words. Never drop a finding to meet a target. + ## 5. De-slop the review Vale catches phrasing, not padding, invented reasons or a comment that argues diff --git a/hack/post_review.py b/hack/post_review.py index 43c2521b1..43aa88eae 100644 --- a/hack/post_review.py +++ b/hack/post_review.py @@ -55,9 +55,10 @@ # marker. MAX_BODY = 65_000 -# The skill limits a comment's prose, outside any code block, to this many -# characters. A sketch of an alternative can run longer. -MAX_PROSE = 600 +# The skill aims for a comment's prose, outside any code block, to stay under +# this many characters. A comment over it is posted anyway, since a long +# comment beats a lost finding, but logged so the skill can be tuned. +TARGET_PROSE = 600 # The review's last line is a marker recording the commit it covered, and the # next review reads it to scope itself. So the agent can't write one, the @@ -130,8 +131,8 @@ def parse_comment(raw: str) -> Comment: raise ValueError(f"comment needs a path, a line and a body: {match['meta']!r}") if len(body) > MAX_COMMENT: raise ValueError(f"comment on {path}:{line} is longer than {MAX_COMMENT} characters") - if len(CODE_BLOCK.sub("", body).strip()) > MAX_PROSE: - raise ValueError(f"comment on {path}:{line} has more than {MAX_PROSE} characters of prose") + if (n := len(CODE_BLOCK.sub("", body).strip())) > TARGET_PROSE: + sys.stderr.write(f"comment on {path}:{line} has {n} characters of prose, over the {TARGET_PROSE} target\n") return Comment(path, int(line), body) From 3802fb5e2bc2f435245ffa46e9988d94bde4783c Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Mon, 2 Nov 2026 11:53:18 -0800 Subject: [PATCH 11/13] Describe the skills without saying who they came from The skills said they were distilled from my review comments and documents, and quoted my corrections to agents as evidence for their rules. Who a rule came from doesn't help an agent follow it, and it makes shared skills read as one maintainer's preferences. Each rule now stands on its own, and the calibration examples name the role rather than the person. Signed-off-by: Nic Cope --- .../skills/design-docs/references/voice.md | 12 ++-- .../references/example.md | 6 +- .agents/skills/reviewing-designs/SKILL.md | 4 +- .../skills/reviewing-pull-requests/SKILL.md | 3 +- .../references/calibration.md | 10 +-- .agents/skills/writing-style/SKILL.md | 64 ++++++------------- 6 files changed, 33 insertions(+), 66 deletions(-) diff --git a/.agents/skills/design-docs/references/voice.md b/.agents/skills/design-docs/references/voice.md index d222bc6ec..b47633429 100644 --- a/.agents/skills/design-docs/references/voice.md +++ b/.agents/skills/design-docs/references/voice.md @@ -1,7 +1,6 @@ # Voice and Structure -What the target looks like, drawn from ~30 of Nic Cope's documents spanning -2019-2025. Read this when drafting from scratch. The failure modes in SKILL.md +What the target looks like. Read this when drafting from scratch. The failure modes in SKILL.md matter more, because they describe where drafts actually go wrong. This file describes where they should end up. @@ -48,20 +47,19 @@ a bit like a Makefile backed by a snapshot of nixpkgs." ### Release verbs point forward -Nic uses "ship" for the act of releasing, as intent or as something still ahead. +Use "ship" for the act of releasing, as intent or as something still ahead. "We need to ship X." "Should v0.1 ship with something simpler?" "Biasing toward shipping rather than another review round." -He never uses it to describe what a product has or does. "Modelplane ships a +Don't use it to describe what a product has or does. "Modelplane ships a Traefik gateway" and "nobody ships this layer today" are LLM phrasings. The fix is the specific verb: Modelplane *installs* a Traefik gateway, *includes* the connector runtime, *composes* the HTTPRoute. Naming the real action is more precise anyway, which is what the linter asks for when it flags the word. -Frequency isn't the problem, so don't overcorrect into avoiding the word. Nic -uses it about as often as an LLM does. The difference is the direction it -points. +Frequency isn't the problem, so don't overcorrect into avoiding the word. The +difference is the direction it points. The same test applies to any verb of delivery or provision with a product as its subject. Say what the software does, and reserve the release verb for releases. diff --git a/.agents/skills/python-code-factoring/references/example.md b/.agents/skills/python-code-factoring/references/example.md index 742d45889..77f2aa70c 100644 --- a/.agents/skills/python-code-factoring/references/example.md +++ b/.agents/skills/python-code-factoring/references/example.md @@ -1,8 +1,8 @@ # Worked Examples -Real before and after pairs from composition function code, taken from the -sessions where these rules got made. The "before" in each case is what an agent -wrote and Nic corrected, so treat the left column as the thing to avoid. +Real before and after pairs from composition function code. The "before" in +each case is what an agent wrote and a maintainer corrected, so treat the left +column as the thing to avoid. Identifiers are as they appeared, minus anything environment-specific. diff --git a/.agents/skills/reviewing-designs/SKILL.md b/.agents/skills/reviewing-designs/SKILL.md index 5c8427e74..9679efb1a 100644 --- a/.agents/skills/reviewing-designs/SKILL.md +++ b/.agents/skills/reviewing-designs/SKILL.md @@ -10,9 +10,7 @@ description: >- # Reviewing Designs -This covers whether a proposal is good, not whether it's well written. It's -distilled from how Nic Cope questions designs, and from the design concerns -Modelplane's maintainers raised on design PRs and what came of them. +This covers whether a proposal is good, not whether it's well written. Maintainers own design decisions. As a reviewer, ask the questions a maintainer would ask and leave the answers to them, since you're better at diff --git a/.agents/skills/reviewing-pull-requests/SKILL.md b/.agents/skills/reviewing-pull-requests/SKILL.md index 7c8980d88..cb98d646a 100644 --- a/.agents/skills/reviewing-pull-requests/SKILL.md +++ b/.agents/skills/reviewing-pull-requests/SKILL.md @@ -10,8 +10,7 @@ description: >- # Reviewing Pull Requests This covers what Modelplane's maintainers look for in a PR and how they phrase -it. It's distilled from about 4,500 of Nic Cope's review comments on -Crossplane and Modelplane, and from the findings he declined. +it. CONTRIBUTING.md wins wherever it disagrees with this skill. Don't spend comments on anything CI already enforces: ruff, ty, Vale on the docs site, the diff --git a/.agents/skills/reviewing-pull-requests/references/calibration.md b/.agents/skills/reviewing-pull-requests/references/calibration.md index 78007aa99..a4f7c6459 100644 --- a/.agents/skills/reviewing-pull-requests/references/calibration.md +++ b/.agents/skills/reviewing-pull-requests/references/calibration.md @@ -6,8 +6,8 @@ about as often as they accept them, and for consistent reasons. ## Posted and accepted -Dennis on #556, which added an early return that skipped composing the cloud -cluster: +A maintainer on #556, which added an early return that skipped composing the +cloud cluster: > **Blocking: when this gate fires on a running cluster, Crossplane deletes the @@ -19,10 +19,10 @@ cluster: The comment gives the consequence and the code path that causes it, then -shows it happening in a test run. Nic raised the same problem independently on -the same PR. +shows it happening in a test run. Another maintainer raised the same +problem independently on the same PR. -Nic on #346: +A maintainer on #346: > I don't think this comment is true - notably nothing seems to read the diff --git a/.agents/skills/writing-style/SKILL.md b/.agents/skills/writing-style/SKILL.md index e31fa5a53..f34cffa06 100644 --- a/.agents/skills/writing-style/SKILL.md +++ b/.agents/skills/writing-style/SKILL.md @@ -5,10 +5,8 @@ description: Write and review prose in Modelplane's style. Use when drafting or # Writing Style -Modelplane's prose follows the style of Nic Cope, its lead maintainer. The -maintainers read every word and delete whatever the reader doesn't need, so aim -for a draft they'd leave as it is. The quotes below are Nic's corrections to -agents, kept as evidence for each rule. +Modelplane's maintainers read every word and delete whatever the reader doesn't +need, so aim for a draft they'd leave as it is. Most of this skill covers what goes wrong. An LLM's instinct for prose is roughly right about voice, and badly wrong about length, honesty about @@ -70,19 +68,10 @@ background section. If you can't find a real reason, say so and ask. Your first draft runs about 40% longer than it should. That's predictable enough to treat as a step in the process. Draft, cut 30%, then read it again and remove -the 10-20% still there. Nic's calibration, verbatim: - - -> Even when I ask you to cut 30% of what you write I can easily find another -> 10-20% that can be cut with zero loss. - +the 10-20% still there. Do both passes before anyone reads it. A draft the reader has to cut costs them -the time the document should have saved, and Nic has said plainly that -repairing LLM prose is expensive: - -> Generally when you write a document it costs me a lot of time spent getting it -> to an actually good state. +the time the document should have saved. Delete first: @@ -111,20 +100,11 @@ answer succinctly" is a coherent instruction. ### Write for the Reader, Not Against the Conversation -Nic makes this correction more sharply than any other, and you won't infer it, -because from inside the session the reactive version feels responsive. - - -> You're once again doing the thing where: A) you write a doc, B) I criticize a -> point, C) you edit the doc to specifically react to that point, explicitly -> naming the point (without reason) throughout. For example your doc now -> litigates what requirements are due to fleet and what aren't, when the reader -> doesn't actually care. - -> This is garbage writing. You're writing this section in reaction to the -> discussion we just had. Think about the audience. Think about the flow of the -> document. Rewrite the section from first principles. - +Maintainers make this correction more sharply than any other, and you won't +infer it, because from inside the session the reactive version feels +responsive. A reviewer criticises a point, and the next draft reacts to it, +naming the point throughout. The document now litigates something its reader +doesn't care about. It appears two ways. The document argues with a reviewer's objection, or it argues with its own earlier draft. Phrasings like "this replaces an earlier @@ -151,8 +131,7 @@ rename without saying why. A plausible reason invented to fill the gap is worse than no reason, because the reader can't tell it apart from a real one and it becomes part of the permanent record. -If you don't know why, ask, or leave the reason out. When Nic catches an -invented reason, the response is blunt: "You just made that all up." +If you don't know why, ask, or leave the reason out. This extends past rationale: @@ -189,8 +168,8 @@ and them, and rhetorical absolutes rarely survive scrutiny. ### Space Signals Importance The room a topic gets is how a reader judges its importance, so proportion -is a property to check. Nic checks it by eye and by line count: "Your new -section takes up more space than any other in the doc. Should it?" +is a property to check, by eye and by line count. If a new section takes up +more space than any other in the document, should it? - Sections stay proportional to their importance to the whole document, never to how recently they came up. Recency bias is a defect. A commit message follows @@ -276,13 +255,7 @@ Grammar gets its own pass. Run-on sentences are the recurring offender. ## The Pass Before You Hand It Back -Nobody should spend reading time on an un-de-slopped draft. Nic, to an agent: - - -> I want you to tackle the prose cuts and AI tells edits before I spend time -> reading. This should be a tight design that reads like I wrote it. No slop, no -> filler. - +Nobody should spend reading time on an un-de-slopped draft. Getting there takes two passes, which catch different things. @@ -303,10 +276,10 @@ vale sync vale path/to/doc.md ``` -**Fix what it reports.** Nic compared 16 linter-driven rewrites against the -originals across two design docs. He preferred the rewrite in 14 and called the -other two a tie. He never preferred the original. His summary: the edits drive -toward more succinct and direct phrasing, and that's the project style. +**Fix what it reports.** A maintainer compared 16 linter-driven rewrites against +the originals across two design docs, and preferred the rewrite in 14, called +the other two a tie, and never preferred the original. The edits drive toward +more succinct and direct phrasing, which is the project style. So comply by default, and override only when you can state the specific reason the alert is wrong. @@ -316,8 +289,7 @@ one direction: you will defend phrasing you wrote. Every alert in that comparison that an agent had confidently called a false positive turned out to be a real catch. The most confident defence was `StackedAnaphora` on "one engine, one member, one pod", argued as deliberate repetition mapping onto the -API. Nic's verdict was that it "reads very AI", and the repetition was the -reason. +API. It read as AI-written, and the repetition was the reason. The exceptions are mechanical rather than matters of taste: From 80e31b799f6fc125a25ff9e6e614927c01a89058 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Mon, 2 Nov 2026 12:03:01 -0800 Subject: [PATCH 12/13] Cap how many steps each review agent can take Nothing stopped a runaway review except the workflow's timeout, and a review that ran to it could spend most of an hour's tokens on one PR. Each agent now has a step cap of about three times the most steps a review has needed. At the cap opencode asks the agent to wrap up rather than failing it, so a long review still posts. Signed-off-by: Nic Cope --- .opencode/agents/review-correctness.md | 1 + .opencode/agents/review-craft.md | 1 + .opencode/agents/review-design.md | 1 + .opencode/agents/review-prose.md | 1 + .opencode/agents/review.md | 1 + hack/review-vm/flake.nix | 5 +++++ 6 files changed, 10 insertions(+) diff --git a/.opencode/agents/review-correctness.md b/.opencode/agents/review-correctness.md index 3ee0d68dc..2d3cc6094 100644 --- a/.opencode/agents/review-correctness.md +++ b/.opencode/agents/review-correctness.md @@ -2,6 +2,7 @@ description: >- Review a pull request for correctness bugs, verified by tracing code paths, running tests and reading upstream source. +steps: 220 mode: subagent --- diff --git a/.opencode/agents/review-craft.md b/.opencode/agents/review-craft.md index d31480265..7275a3ea6 100644 --- a/.opencode/agents/review-craft.md +++ b/.opencode/agents/review-craft.md @@ -2,6 +2,7 @@ description: >- Review a pull request's code for craft: whether each addition needs to exist, consistency with the repo, naming, tests and project style. +steps: 140 mode: subagent --- diff --git a/.opencode/agents/review-design.md b/.opencode/agents/review-design.md index b75e03ae8..d22a97ed9 100644 --- a/.opencode/agents/review-design.md +++ b/.opencode/agents/review-design.md @@ -2,6 +2,7 @@ description: >- Review a pull request's premise and design, including any design doc it adds, against its issue and the committed designs. +steps: 90 mode: subagent --- diff --git a/.opencode/agents/review-prose.md b/.opencode/agents/review-prose.md index 331e83abc..afde9dd31 100644 --- a/.opencode/agents/review-prose.md +++ b/.opencode/agents/review-prose.md @@ -2,6 +2,7 @@ description: >- Review a pull request's prose: comments, docstrings, XRD field descriptions, docs, design docs, the PR description and commit messages. +steps: 130 mode: subagent --- diff --git a/.opencode/agents/review.md b/.opencode/agents/review.md index f798fbb8b..18b3472cb 100644 --- a/.opencode/agents/review.md +++ b/.opencode/agents/review.md @@ -3,6 +3,7 @@ description: >- Review a Modelplane pull request. Runs the design, prose, craft and correctness reviewers in parallel, then posts only the findings that hold up. +steps: 120 mode: primary --- diff --git a/hack/review-vm/flake.nix b/hack/review-vm/flake.nix index a534a78c1..c587c2fad 100644 --- a/hack/review-vm/flake.nix +++ b/hack/review-vm/flake.nix @@ -67,6 +67,11 @@ # The review script adds the model and its provider, which the # workflow passes in model.json. autoupdate = false; + # The review agents cap their own steps. This caps the de-slop + # pass, which runs as the built-in general agent. Every cap is + # about three times the most steps a review has needed, to stop + # a runaway without cutting a long review short. + agent.general.steps = 50; plugins = [ "${lspPlugin}" ]; lsp.vale = { command = [ "vale-ls" ]; From 369305e49be5cdde205c9a867f90155b15896242 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Mon, 2 Nov 2026 12:24:58 -0800 Subject: [PATCH 13/13] Footnote each review with the reviewer's version Reactions and replies to review comments are the cheapest signal of whether a skill change helped, but nothing recorded which version of the skills and agents wrote a comment. Each review now ends with a footnote naming the commit the reviewer came from, and asking readers to react to each comment. Signed-off-by: Nic Cope --- .github/workflows/review.yml | 1 + hack/post_review.py | 23 +++++++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 86b5c21c3..6acd615d2 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -342,4 +342,5 @@ jobs: LAST: ${{ needs.review.outputs.last }} run: | python3 hack/post_review.py --repo "$GITHUB_REPOSITORY" --pr "$PR" --sha "$SHA" \ + --reviewer "$(git rev-parse HEAD)" \ ${LAST:+--since "$LAST" --git-dir .git} out.tar diff --git a/hack/post_review.py b/hack/post_review.py index 43aa88eae..8d7c83ba6 100644 --- a/hack/post_review.py +++ b/hack/post_review.py @@ -26,8 +26,9 @@ the summary instead. The summary ends with a marker recording the commit the review covered, which the next review reads to review only what's new. The summary keeps to its 1,000 character limit by dropping later paragraphs. A -review from an agent that didn't exit cleanly may be incomplete, so it gets no -marker. +footnote ahead of the marker names the commit the reviewer's skills and agents +came from, and asks readers to react to each comment. A review from an agent +that didn't exit cleanly may be incomplete, so it gets no marker. A re-review passes --since, the commit the last review covered. Its inline comments may then only target lines the PR adds that also changed since that @@ -216,7 +217,11 @@ def changed(diff: str) -> set[int]: def place( - review: Review, diff: dict[str, set[int]], sha: str, scope: dict[str, set[int]] | None = None + review: Review, + diff: dict[str, set[int]], + sha: str, + scope: dict[str, set[int]] | None = None, + footnote: str = "", ) -> tuple[str, list[Comment]]: """The review body and the comments GitHub accepts inline. Without a scope, a comment on a line outside the diff moves into the body. With one, only @@ -238,6 +243,8 @@ def place( body += moved for c in outside if scope is not None else []: sys.stderr.write(f"dropping a comment on {c.path}:{c.line}, which hasn't changed since the last review\n") + if footnote: + body += "\n\n" + footnote # Without the marker, the next review covers the whole PR rather than # skipping work an incomplete run never finished. if review.complete: @@ -261,6 +268,7 @@ def main() -> int: parser.add_argument("--repo", required=True) parser.add_argument("--pr", required=True) parser.add_argument("--sha", required=True) + parser.add_argument("--reviewer", required=True, help="the commit the reviewer's skills and agents came from") parser.add_argument("--since", help="the commit the last review covered, for a re-review") parser.add_argument("--git-dir", type=pathlib.Path, help="a repository holding --sha and --since") parser.add_argument("output", type=pathlib.Path, help="the tar the review VM wrote") @@ -301,7 +309,14 @@ def main() -> int: ).stdout scope[f["filename"]] = added(f.get("patch", "")) & changed(since) - body, inline = place(review, diff, args.sha, scope) + # Which reviewer wrote a comment tells the reactions to it apart by skill + # version, when they're harvested later. + footnote = ( + f"Reviewed by modelplane-review [`{args.reviewer[:7]}`]" + f"(https://github.com/{args.repo}/commit/{args.reviewer}). " + "Reply, or react 👍 or 👎, on each comment to help tune it." + ) + body, inline = place(review, diff, args.sha, scope, footnote) payload = { "commit_id": args.sha, "event": "COMMENT",