From 2c67a9e75446d6a91544483d488dc3b92c0887a8 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 02:44:02 -0600 Subject: [PATCH 1/4] Refactor: Compact the constable skill and group it by task The skill mixed the run mechanics with the contract rules, and it spread one rule across several paragraphs, so an agent had to read all of it to find the rule that its code broke. This restructure is a prerequisite for the repair advice that follows: the advice needs one place per topic to point at. Group the text as run mechanics, the annotation, the reported writes, the allowed writes, predicates, workflow, and limits. Consolidate the five mutation examples into two that carry the same facts, state the exit statuses and the flags that the agent needs, and drop the prose that repeated the README. Change-Id: I94c6c634e4947d3428e1d94db721e1405b8a1881 --- skills/constable/SKILL.md | 147 +++++++++++++++----------------------- 1 file changed, 59 insertions(+), 88 deletions(-) diff --git a/skills/constable/SKILL.md b/skills/constable/SKILL.md index c415843..97d9bd1 100644 --- a/skills/constable/SKILL.md +++ b/skills/constable/SKILL.md @@ -6,153 +6,124 @@ description: Use constable to check that annotated Go functions and methods do n # Go Constable `constable` is a Go static analysis tool for opt-in non-mutation checks. - -Use it when a function or method should read its inputs without changing state that the caller can observe. +Use it when a function or method must read its inputs without changing state that the caller can observe. The check does not guarantee that the function is pure or free of all side effects. ## Installation and invocation -Install the released command with: +Install the released command, then run it from the module root, ```sh go install github.com/nnutter/constable/cmd/constable@latest +constable ./... ``` -Run it against the packages in a module: +Each diagnostic has the form `file:line:col: message` and goes to stderr. +Exit status `0` means no diagnostic, `3` means at least one diagnostic, and `1` means constable failed. +An invocation that finds no diagnostic succeeds silently. -```sh -constable ./... -``` +Further flags: + +- `-json` prints JSON and always exits `0`. +- `-c N` prints N lines of source around each reported line. +- `-fix` writes the fixes, and `-fix -diff` prints them as a diff and writes nothing. +- `-=false` turns off one analyzer, for example `-complexity=false`. +- `-flags` lists every flag, and `help ` describes one analyzer. -The command reports diagnostics and exits unsuccessfully when an annotated function violates the contract. -An invocation with no diagnostics succeeds silently. +A file that carries the generated-code comment `// Code generated ... DO NOT EDIT.` is skipped, because the next run of the generator overwrites it. +Change such a file through its generator. +`-include-generated` reports in it anyway. ## Annotating Go code Put the exact directive `//constable:nonmutating` immediately before the function or method declaration. -There must not be a space between `//` and `constable:`. +Do not put a space between `//` and `constable:`. ```go //constable:nonmutating func ReadValue(p *int) int { - return *p + return *p } ``` The directive applies to functions and methods only. -Do not add it to a function that is intentionally allowed to mutate caller-visible state. - -## Mutations that must be avoided +Do not add it to a function that must change caller-visible state. -The analyzer reports direct mutations through pointer, slice, and map parameters. -It also reports mutations through pointer receivers and through reference-like fields of value receivers. - -```go -//constable:nonmutating -func WriteValue(p *int) { - *p = 1 // reported -} - -//constable:nonmutating -func WriteSlice(values []int) { - values[0] = 1 // reported -} +## Reported mutations -//constable:nonmutating -func WriteMap(values map[string]int) { - values["key"] = 1 // reported -} +The analyzer reports these writes: -//constable:nonmutating -func DeleteMapEntry(values map[string]int) { - delete(values, "key") // reported -} +- A write through a pointer parameter or a pointer receiver, such as `*p = 1`. +- A write to a slice element, a map entry, or a field that the parameter reaches. +- `delete` on a map parameter. +- A write to a reference-like field of a value receiver, such as a slice field. +- The same writes through an alias that a local variable holds. -type Buffer struct { - data []byte -} +```go +type Buffer struct{ data []byte } //constable:nonmutating func (b Buffer) ClearFirstByte() { - b.data[0] = 0 // reported: b.data shares caller-visible backing storage + b.data[0] = 0 // b.data shares the caller's backing array } ``` -Aliases of parameters are also checked when the alias is assigned to a local variable. -Nested reference-like values, such as a slice of slices, are considered caller-visible when mutated. +Nested references count too, such as a write to an element of a slice of slices. -## Code that is allowed +## Allowed code -Reassigning a parameter does not mutate the value held by the caller. -Mutating a local variable or a pointer to a local variable is also allowed. -A value receiver may mutate its scalar fields because those fields belong to the receiver copy. +- A new value for the parameter itself, such as `p = nil`. +- A write to a local variable, or to memory that the function allocates. +- A scalar field write on a value receiver, because that field belongs to the receiver copy. ```go -//constable:nonmutating -func ReassignParameter(p *int) { - p = nil // allowed -} - -//constable:nonmutating -func MutateLocal() int { - value := 0 - value++ // allowed - return value -} - -type Count struct { - n int -} +type Count struct{ n int } //constable:nonmutating func (c Count) Increment() Count { - c.n++ // allowed: c is a value-receiver copy - return c + c.n++ // c is the receiver copy + return c } ``` ## Predicates -The predicates analyzer reports `&&` and `||` operands, except the first, that are not predicate-function calls. -The first operand always executes, so coverage observes short-circuiting through the rest. +The predicates analyzer reports `&&` and `||` operands, except the first, that are not calls to a predicate function. +The first operand always runs, so coverage observes short-circuiting through the operands that follow. ```go func F(a, b bool) { - if a && b { // reported: use a && isB() - } + if a && b { // reported: use a && isB() + } } func isB() bool { return false } func G(a bool) { - if a && isB() { // allowed - } + if a && isB() { // allowed + } } ``` -The check covers `if` and `for` conditions and bool assignments such as `c := a && b`, including `!a`, field and map access, and comparisons. -Operands that are already calls, compound expressions, or `true`/`false`/`nil` are allowed. - -## Agent workflow +The check covers `if` and `for` conditions and bool assignments such as `c := a && b`. +It accepts an operand that is a call, a compound expression, a negated call, or `true`, `false`, or `nil`. -1. Identify functions and methods whose API contract should not mutate caller-owned data. -1. Add `//constable:nonmutating` immediately before each eligible declaration. -1. Run `constable ./...` from the Go module root. -1. For every diagnostic, either remove the caller-visible mutation or remove the annotation if the contract is incorrect. -1. Re-run the analyzer and the package tests before finalizing the change. +## Workflow -Prefer returning a new value or making a defensive copy when the implementation needs to transform caller-provided data. -Do not silence a diagnostic by merely reassigning a parameter while continuing to mutate data reachable through it. +1. Identify the functions and methods whose contract must not change caller-owned data. +1. Add `//constable:nonmutating` immediately before each of them. +1. Run `constable ./...`. +1. For each diagnostic, remove the caller-visible mutation, or delete the directive when the contract is wrong. +1. Re-run constable and the package tests before you finalize the change. -## Scope and limitations +Prefer a new return value or a defensive copy when the implementation must transform caller-provided data. +Do not hide a diagnostic by assigning a new value to the parameter while the code still writes through it. -The check is focused on direct, intra-procedural mutations represented by assignments, increments, slice or map indexing, and `delete`. -It is not a general immutability checker. -It does not by itself guarantee that a function has no I/O, global state changes, goroutines, reflection, unsafe operations, or other side effects. -Calls to helper functions, interface methods, function values, and other indirect mutation paths may not be modeled. -The analyzer currently recognizes only the `nonmutating` directive. +## Scope and limits -The analyzers skip files that carry the standard generated-code comment, `// Code generated ... DO NOT EDIT.`. -Edit such a file through its generator, not by hand. +The check finds direct writes in one function body: assignment, `++`, `--`, index writes, field writes, and `delete`. +It does not model a call to a helper function, a write through `append`, or an indirect path such as an interface method or a function value. +The analyzer recognizes only the `nonmutating` directive, and it is not a general immutability checker. +It does not by itself guarantee that a function has no I/O, global state changes, goroutines, reflection, or unsafe operations. -When behavior is uncertain, inspect the diagnostic and the implementation rather than assuming that a clean run proves complete immutability. +When a run is clean, inspect the implementation before you claim that it is fully non-mutating. From 1e00c5905a68b5937767ac828dcc8a8db588cc53 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 02:45:17 -0600 Subject: [PATCH 2/4] Document a repair for each diagnostic from each analyzer An agent that reads a diagnostic needs the recovery step, not only the rule that the code broke. The skill described two of the nine analyzers and gave no repair for either, so the agent had to read the analyzer source or guess, and it never learned that constable also reports method placement, test testify use, complexity, unchecked errors, missing sum type cases, and nil flows. Add a repair subsection per analyzer. Each one lists the message text that the agent will see, so the agent can match the message to the repair, and then states the repair: - nonmutating: copy the data or return a new value; delete the directive only when the contract is wrong, and never reassign the parameter to hide the message. The message names the origin kind, and the writes that count are in the contract section below it. - methodical: run `-fix`, which moves and orders the methods, and which files it skips. Move the method by hand when the fix skips it. - predicates: move the operand into a function; a variable does not satisfy the analyzer. - testify: `assert` replaces a call that continues the test, and `require` replaces a call that stops it. - complexity: extract branches, and state the count so the agent can predict the next diagnostic. - errcheck, sumtype, testifylint, nilaway: a table of message patterns and repairs, plus the NilAway scope flags and the `//nolint:nilaway` suppression. Also record the failure exit status, because an agent that sees `analysis skipped due to errors in package` must repair the build before it reads any diagnostic. Update the description to say that the skill covers the analyzers and their repairs. Change-Id: Iedb61f32dd80f4c5dbf1368768d68d25e5152b3c --- skills/constable/SKILL.md | 168 ++++++++++++++++++++++++++++---------- 1 file changed, 126 insertions(+), 42 deletions(-) diff --git a/skills/constable/SKILL.md b/skills/constable/SKILL.md index 97d9bd1..549c332 100644 --- a/skills/constable/SKILL.md +++ b/skills/constable/SKILL.md @@ -1,15 +1,15 @@ --- name: constable -description: Use constable to check that annotated Go functions and methods do not mutate caller-visible argument or receiver state. +description: Use constable to check Go packages with its analyzers and to repair each diagnostic that it reports. --- # Go Constable -`constable` is a Go static analysis tool for opt-in non-mutation checks. -Use it when a function or method must read its inputs without changing state that the caller can observe. -The check does not guarantee that the function is pure or free of all side effects. +`constable` is a static analysis tool for Go. +It runs a set of analyzers on each package and prints one diagnostic per line. +Use it to check a change, and to hold a repository rule in a hook or in CI. -## Installation and invocation +## Run it Install the released command, then run it from the module root, @@ -34,10 +34,122 @@ A file that carries the generated-code comment `// Code generated ... DO NOT EDI Change such a file through its generator. `-include-generated` reports in it anyway. -## Annotating Go code +An exit status `1` with `analysis skipped due to errors in package` means the package does not build. +Fix the build error first, then run constable again. -Put the exact directive `//constable:nonmutating` immediately before the function or method declaration. -Do not put a space between `//` and `constable:`. +## Repair each diagnostic + +### nonmutating + +```text +//constable:nonmutating function mutates pointer parameter p +//constable:nonmutating function mutates parameter s +//constable:nonmutating function deletes from map parameter m +//constable:nonmutating method mutates receiver r +``` + +The statement writes to data that the caller owns. +The non-mutating contract section below lists the writes that count. +The message names the origin: `pointer parameter` for a write through a pointer, `parameter` for a slice, a map, or another reference, `receiver` for a method receiver, and `deletes` for `delete` on a map. +Repair it in this order: + +1. Stop the write. + Copy the data and write to the copy, or return a new value for the caller to assign. +1. Delete the directive when the function must change caller-visible state, because the contract is wrong for that function. + +`p = nil` on its own reports nothing, but the caller's data still changes when the code also writes through `p`. +Do not hide a diagnostic by assigning a new value to the parameter. +Prefer the copy or the new value. + +### methodical + +```text +method Read of type Buffer should be in same file as type definition +method Append of type Buffer should be sorted before method Read +method Len of type Buffer should be grouped with method Read +``` + +Run the fix, + +```sh +constable -fix -diff ./... # read the change +constable -fix ./... # write the change +``` + +The fix moves every method of the reported type directly after the type declaration. +It orders those methods by name, with a generic method after the non-generic methods. +It adds the imports that a moved method needs and removes the imports that the old file no longer uses. + +Move a method by hand when the fix skips it. +The fix skips a generated file, a file with build constraints or Go directives, a file that uses cgo, a file whose import block holds comments, a file with comments that no declaration owns, and a declaration that does not start its own line. +The fix writes only a file that Git tracks and that has no changes, so `git checkout` recovers the file. + +### predicates + +```text +use predicate function instead of "b" in compound conditional +``` + +The quoted text is the operand to replace. +Move that operand into a function that returns `bool`, and call the function in its place, + +```go +func isB() bool { return b } + +if a && isB() { // was: if a && b { +``` + +A variable does not satisfy the analyzer. +Only a call does. + +### testify + +```text +use testify/assert instead of testing.Error +use testify/require instead of testing.Fatal +``` + +Replace the `testing` call with the testify call, and add the import. +`assert` does not stop the test, so it replaces `t.Error`, `t.Errorf`, and `t.Fail`. `require` stops the test, so it replaces `t.Fatal`, `t.Fatalf`, and `t.FailNow`. +Prefer a specific assertion, such as `assert.Equal` or `require.NoError`, because it prints the values that differ. + +### complexity + +```text +cyclomatic complexity 12 exceeds limit 10 (function Busy) +``` + +Extract branches into smaller functions until the count is at or below the limit. +The count starts at 1 and adds 1 for each `if`, `for`, `range`, `case`, and `select` clause, `&&`, and `||`. +A closure adds to the function that holds it, and a `default` clause counts. +Raise the limit with `-complexity.limit=15` only when the function must keep its shape, and say why in the change. + +### errcheck, sumtype, testifylint, and nilaway + +These analyzers come from other modules. +Their messages, flags, and repairs are theirs. + +| Analyzer | Message | Repair | +| --- | --- | --- | +| `errcheck` | `unchecked error` | Handle the error. Assign it to `_` only when the code discards it on purpose. | +| `sumtype` | `missing cases for Square` | Add the case for the variant, or add a `default` clause and set `-sumtype.default-signifies-exhaustive`. | +| `testifylint` | text that names a testify check | Follow the message. The check name states the misuse, such as `expected-actual`, `formatter`, or `require-error`. | +| `nilaway` | `Potential nil panic detected` | Remove the nil flow: check the value, return early, or initialize it where it is declared. | + +The embedded analyzers have more flags than this document lists. +Their flags keep the analyzer name as a prefix, such as `-errcheck.blank` or `-testifylint.disable-all`. + +NilAway reads every package that it loads by default, which costs seconds and gigabytes. +constable limits it to the main modules of the packages that it analyzes. +Set `-nilaway_config.include-pkgs=example.com/my/module` to choose the packages, or `-nilaway.include-all-pkgs` to analyze the dependencies and the standard library too. +Suppress a report that the code cannot fix with a `//nolint:nilaway` comment on the reported statement. + +## The non-mutating contract + +`//constable:nonmutating` states that the function or method does not change state that the caller can observe through its arguments or its receiver. +The check does not prove that the function is pure or free of side effects. + +Put the exact directive immediately before the declaration, with no space between `//` and `constable:`. ```go //constable:nonmutating @@ -47,11 +159,8 @@ func ReadValue(p *int) int { ``` The directive applies to functions and methods only. -Do not add it to a function that must change caller-visible state. - -## Reported mutations -The analyzer reports these writes: +### Reported mutations - A write through a pointer parameter or a pointer receiver, such as `*p = 1`. - A write to a slice element, a map entry, or a field that the parameter reaches. @@ -70,7 +179,7 @@ func (b Buffer) ClearFirstByte() { Nested references count too, such as a write to an element of a slice of slices. -## Allowed code +### Allowed code - A new value for the parameter itself, such as `p = nil`. - A write to a local variable, or to memory that the function allocates. @@ -86,44 +195,19 @@ func (c Count) Increment() Count { } ``` -## Predicates - -The predicates analyzer reports `&&` and `||` operands, except the first, that are not calls to a predicate function. -The first operand always runs, so coverage observes short-circuiting through the operands that follow. - -```go -func F(a, b bool) { - if a && b { // reported: use a && isB() - } -} - -func isB() bool { return false } - -func G(a bool) { - if a && isB() { // allowed - } -} -``` - -The check covers `if` and `for` conditions and bool assignments such as `c := a && b`. -It accepts an operand that is a call, a compound expression, a negated call, or `true`, `false`, or `nil`. - ## Workflow 1. Identify the functions and methods whose contract must not change caller-owned data. 1. Add `//constable:nonmutating` immediately before each of them. 1. Run `constable ./...`. -1. For each diagnostic, remove the caller-visible mutation, or delete the directive when the contract is wrong. +1. Repair each diagnostic with the advice above. 1. Re-run constable and the package tests before you finalize the change. -Prefer a new return value or a defensive copy when the implementation must transform caller-provided data. -Do not hide a diagnostic by assigning a new value to the parameter while the code still writes through it. - ## Scope and limits -The check finds direct writes in one function body: assignment, `++`, `--`, index writes, field writes, and `delete`. +The non-mutating check finds direct writes in one function body: assignment, `++`, `--`, index writes, field writes, and `delete`. It does not model a call to a helper function, a write through `append`, or an indirect path such as an interface method or a function value. -The analyzer recognizes only the `nonmutating` directive, and it is not a general immutability checker. -It does not by itself guarantee that a function has no I/O, global state changes, goroutines, reflection, or unsafe operations. +It is not a general immutability checker, and it does not by itself guarantee that a function has no I/O, global state changes, goroutines, reflection, or unsafe operations. +The analyzer recognizes only the `nonmutating` directive. When a run is clean, inspect the implementation before you claim that it is fully non-mutating. From 3fb9c1bcc7b12cfdc07ddf1a190d1a25a56a8774 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 02:51:22 -0600 Subject: [PATCH 3/4] Remove the install step from the constable skill The step told the agent to install `constable@latest`, which can leave a version that differs from the one the repository uses: the hook builds the working tree, and CI installs the release that the repository pins. The skill now assumes that the command is on the PATH, as the hook and CI provide it, and it starts at the invocation that the agent must run. Change-Id: Ie5dac101271d677d33833bc2bdd48e97591a9285 --- skills/constable/SKILL.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/skills/constable/SKILL.md b/skills/constable/SKILL.md index 549c332..692df2a 100644 --- a/skills/constable/SKILL.md +++ b/skills/constable/SKILL.md @@ -11,10 +11,9 @@ Use it to check a change, and to hold a repository rule in a hook or in CI. ## Run it -Install the released command, then run it from the module root, +Run it from the module root, ```sh -go install github.com/nnutter/constable/cmd/constable@latest constable ./... ``` From e51f5e4f05924f9586bd44492e93d5ae2eb23b6f Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 02:51:53 -0600 Subject: [PATCH 4/4] Advise a domain name before extracting a branch The complexity advice said only to extract branches until the count is at or below the limit. An agent can satisfy that rule at any seam: cut the function where the count drops, name the piece after the steps it takes, and leave the reader with two lists of steps and no rule. Add the test that separates a branch worth extracting from a seam that only moves code: the new function needs a name from the problem domain, such as `isOverdue` or `hasOpenInvoice`. A name that restates the steps, such as `checkFlagsAndReturnTwo`, does not help the reader, and a branch that admits only that kind of name is not a good candidate. The agent then splits the function somewhere else, or raises the limit when the function is one coherent rule. State that `-complexity.limit` applies to the whole run, so the agent does not read the earlier sentence as a per-function setting. Change-Id: Icb767816eac9926bcad29f8e6fb656fcc1e0d413 --- skills/constable/SKILL.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/skills/constable/SKILL.md b/skills/constable/SKILL.md index 692df2a..56e9ed8 100644 --- a/skills/constable/SKILL.md +++ b/skills/constable/SKILL.md @@ -119,9 +119,16 @@ cyclomatic complexity 12 exceeds limit 10 (function Busy) ``` Extract branches into smaller functions until the count is at or below the limit. +Extract a branch when you can give the new function a name from the problem domain, such as `isOverdue` or `hasOpenInvoice`. +That name states the fact that the condition tests, so the reader sees the rule instead of the steps. +A name that only restates the steps, such as `checkFlagsAndReturnTwo`, does not help the reader. +When only that kind of name fits, the branch is not a good candidate for extraction. +Split the function somewhere else. +When the function is one coherent rule, raise the limit instead. The count starts at 1 and adds 1 for each `if`, `for`, `range`, `case`, and `select` clause, `&&`, and `||`. A closure adds to the function that holds it, and a `default` clause counts. -Raise the limit with `-complexity.limit=15` only when the function must keep its shape, and say why in the change. +`-complexity.limit=15` raises the limit for the whole run. +Say why in the change when you use it. ### errcheck, sumtype, testifylint, and nilaway