Share general review and documentation rules

[onechat] / .agents / skills / go-code / SKILL.md

diff --git a/.agents/skills/go-code/SKILL.md b/.agents/skills/go-code/SKILL.md
index 086360ad7319aa80fa2e8ea4f8ce53940ad6db3b..59fd54453a676105491a0bfb95695a26af4afa76 100644 (file)
--- a/.agents/skills/go-code/SKILL.md
+++ b/.agents/skills/go-code/SKILL.md
@@ -24,10 +24,18 @@ description: Go style rules. Use before you write or review Go code.
   case.
 - Write all the letters of an initialism in one case. `revive` checks
   common initialisms.
   case.
 - Write all the letters of an initialism in one case. `revive` checks
   common initialisms.

-- Put entry points first, then each struct's methods as a group, then
-  standalone utility functions. Do not put a utility function between
-  the methods of one type.
-- In each group, order functions by first use in the file.
+- Order declarations as imports, constants, variables, types,
+  constructors, methods, then standalone helpers. Types include structs,
+  interfaces, and aliases. Put all types before any constructor or
+  method, also when a file holds more than one type. The Tests section
+  gives the exception for files with tests.
+- Keep each type's methods in one group. Do not put a helper between
+  them. Order functions by first use within their declaration group, not
+  across groups. Entry points do not go before types or constructors
+  merely because they are entry points.
+- Group related constants in one `const (...)` block and related
+  variables in one `var (...)` block. Put an empty line between entries
+  that have comments.

 
 ## Entry point and context
 
 
 ## Entry point and context
 


@@ -40,15 +48,34 @@ description: Go style rules. Use before you write or review Go code.
   Call `defer cancel()` immediately after you create the context.
 - Reuse `ctx` when a child context replaces its parent. Use a different
   name only when both contexts stay in use. Return `ctx.Err()` directly.
   Call `defer cancel()` immediately after you create the context.
 - Reuse `ctx` when a child context replaces its parent. Use a different
   name only when both contexts stay in use. Return `ctx.Err()` directly.

+- Derive each context from the context of the caller. When work must
+  finish even after the caller is cancelled, such as a final write, use
+  `context.WithoutCancel` of the caller's context. Use
+  `context.Background()` only where no caller context exists: in `run`,
+  and in methods that follow `io.Closer`.
+- Name a duration that limits one operation after that operation, and
+  end the name with `Timeout`, so that the name says what it limits
+  without its comment. Define it in the file that applies it, not in the
+  file of another type.

 
 ## Errors
 
 
 ## Errors
 

-- Handle every returned error.
+- Handle every returned error, including an error assigned to `_`. Never
+  use a blank assignment to hide an unchecked error from a linter.
+  Handle meaningful errors. When the user approves an intentional
+  discard, call the operation with an explicit `// nolint: errcheck`
+  directive and a comment that gives the reason, not a blank assignment.
+  The `check` skill gives the approval rule.

 - Reuse an existing `err` for later operations in the same scope. Use a
   different variable only to keep or join errors.
 - Use `errors.Is` and `errors.As` to examine errors. Do not compare an
   error directly with a sentinel, because errors can be wrapped.
   `errorlint` checks comparisons and type assertions on errors.
 - Reuse an existing `err` for later operations in the same scope. Use a
   different variable only to keep or join errors.
 - Use `errors.Is` and `errors.As` to examine errors. Do not compare an
   error directly with a sentinel, because errors can be wrapped.
   `errorlint` checks comparisons and type assertions on errors.

+- Do not match the text of an error from another package. When an
+  external fault leaves no other way, test it through the real code
+  path, so that a change of the text fails a test.
+- Handle a documented sentinel error explicitly when it does not show a
+  failure.

 - When a function has more than one error path, wrap each returned error
   with the context of its operation. A function with one error path can
   return the error directly. `wrapcheck` checks package and interface
 - When a function has more than one error path, wrap each returned error
   with the context of its operation. A function with one error path can
   return the error directly. `wrapcheck` checks package and interface


@@ -57,6 +84,28 @@ description: Go style rules. Use before you write or review Go code.
 - When deferred cleanup can fail, use a named `err` result and join the
   cleanup error with `errors.Join`.
 
 - When deferred cleanup can fail, use a named `err` result and join the
   cleanup error with `errors.Join`.
 

+## Logs and cancellation
+
+- Log with the `*slog.Logger` of the part (`daemonLogger`). The handler
+  formats the line; do not format values into the message.
+- Write the message as a short event in plain words, such as "receive
+  failed" or "subagent done". Do not explain what it means or what the
+  user must do.
+- Put names, IDs, counts, and durations in attributes, with short keys
+  (`name`, `id`, `after`, `retry`). Put the cause in `err`; the handler
+  writes it last.
+- Use info for normal events, warn for a failure that is retried or
+  loses nothing, and error for a failure that loses data or stops a
+  part.
+- Cancellation of a service or request context is an expected stop, not
+  a failure. Do not log it as an error, and do not hide all errors only
+  because the context is canceled. A joined error with an independent
+  failure stays a failure.
+- Use the worker or request context for this check. An operation that
+  reaches its own timeout while its worker is active stays a failure.
+- Never log secrets, tokens, message text, or phone numbers. Redact IDs
+  as the existing code does.
+

 ## Style
 
 - Keep function signatures on one line. Write function bodies on more
 ## Style
 
 - Keep function signatures on one line. Write function bodies on more


@@ -69,11 +118,21 @@ description: Go style rules. Use before you write or review Go code.
   table-driven test. A small type of two fields that acts as one value,
   such as a key or a coordinate pair, can use a positional literal on
   one line.
   table-driven test. A small type of two fields that acts as one value,
   such as a key or a coordinate pair, can use a positional literal on
   one line.

-- Put an empty line between logical blocks, also after multiline
-  declarations, assignments, literals, and calls. Keep an operation with
-  its immediate error or result check. Keep a short declaration with its
-  immediate use. `wsl_v5` and `nlreturn` check part of this rule. Review
-  the remaining logical blocks and immediate-use groups by hand.
+- Put an empty line between logical blocks, such as two independent `if`
+  statements. Put an empty line before and after loops, switches,
+  selects, standalone blocks, and multiline declarations, assignments,
+  literals, and calls, when there is an adjacent statement. These
+  boundaries also apply when a loop immediately uses the declaration
+  before it.
+- The only exception after a multiline construct is its immediate error
+  check. Keep a single-line declaration with its immediate use, and an
+  operation with its immediate error check. A result assertion does not
+  waive the separator after a multiline construct. `wsl_v5` and
+  `nlreturn` check only part of these rules. Review the other boundaries
+  by hand.
+- Group imports in this order: standard library, external dependencies,
+  project imports. Review these groups by hand when the formatter does
+  not check them.

 - Use GNU-style command flags: two dashes for long flags, such as
   `--cwd`, and one dash only for one-letter flags, such as `-h`.
 - Prefer the Go standard library. Prefer Go over cgo.
 - Use GNU-style command flags: two dashes for long flags, such as
   `--cwd`, and one dash only for one-letter flags, such as `-h`.
 - Prefer the Go standard library. Prefer Go over cgo.


@@ -89,17 +148,29 @@ description: Go style rules. Use before you write or review Go code.
 
 ## Tests
 
 
 ## Tests
 

-- Add tests for meaningful behavior. Do not add tests that only repeat
-  implementation details.
+- Add tests for behavior changes. Do not add tests that only repeat
+  implementation details, and do not add a test file without behavior
+  worth a test.

 - Use `testify/require` for assertions. `testifylint` checks the
   assertion forms.
 - Name a table-driven case slice `testCases` and its range variable
   `tc`. A subtest name describes the behavior, not a generic outcome or
   input.
 - Use `testify/require` for assertions. `testifylint` checks the
   assertion forms.
 - Name a table-driven case slice `testCases` and its range variable
   `tc`. A subtest name describes the behavior, not a generic outcome or
   input.

-- In a test file, put constants immediately after the imports, then the
-  tests, then supporting types, methods, and helpers.
+- Each file with tests must have the name of its owning production file
+  with `_test` before `.go`: `memory_test.go` tests `memory.go`. Move
+  scenario and integration tests into their owner's test file. Do not
+  add an empty production file to make a test filename valid. Only
+  `_helper_test.go` files are exempt from this pairing rule.
+- In an ordinary `_test.go` file, order declarations as imports,
+  constants, variables, tests, supporting types, constructors, methods,
+  then helpers. This tests-first order is the exception to the
+  production order. Put all supporting types before their constructors
+  and methods.
+- A `_helper_test.go` file has no tests. Order its declarations as
+  imports, constants, variables, types, constructors, methods, then
+  helpers, as in a production file.

 - Keep a helper that only one test file uses in that file. Put a helper
   that more than one test file uses in a file without tests. Name that
   file for what it holds, and end the name with `_helper_test.go`.
 - Supply valid required dependencies in tests. Do not make a dependency
 - Keep a helper that only one test file uses in that file. Put a helper
   that more than one test file uses in a file without tests. Name that
   file for what it holds, and end the name with `_helper_test.go`.
 - Supply valid required dependencies in tests. Do not make a dependency

-  optional only to make a test simpler.
+  optional or add a nil fallback only to make a test simpler.