AGENTS.md@internal · diff
git:20261019.8ba2204 to git:20261020.8d47d7a
50 added, 77 removed. Audit A to A.
# Agent Instructions — `internal/`
- These instructions apply to `internal/` and may be refined by nearer scoped
- `AGENTS.md` files. Defer to root [`AGENTS.md`](../AGENTS.md) for repo-wide
- rules.
-
## Integration tests share one database
A Go integration test package (`internal/api/integration_test`,
- `internal/storage/stmttest`) brings up one database and runs every test in it.
- Rows a test leaves behind are paid for by every test that runs after it.
-
- That cost is real, not theoretical. The authz list predicate is a correlated
- `EXISTS`, and on the Spanner emulator its cost grows with the rows in the
- tables: reading lists against everything the package had accumulated took one
- test to 687.9s of a 727.8s package.
-
- So a test cleans up what it creates. The cheapest way to get that is to create
- every fixture under the test's own project, because
- `helpers.Harness.EnsureProjectService` registers the delete for you and the
- authz tables (`resource_scope_index`, `authz_assignments`,
- `authz_membership_edges`) cascade from `projects`.
+ `internal/storage/stmttest`) runs every test in one database, so rows a test
+ leaves behind slow every test after it. On the Spanner emulator the authz list
+ predicate's cost grows with the rows in its tables.
- Anything a test writes outside its own project it has to clean up itself, with
- its own `t.Cleanup` and `context.Background()`. Rows written into the platform
- project and the catalog tables are the usual cases. A project created over HTTP
- is another: the handler is cached for the whole run, so it holds the unwrapped
- service, and the test calls `Harness.CleanupProject` for what it created.
+ - Clean up what a test creates. Create fixtures under the test's own project:
+ `helpers.Harness.EnsureProjectService` registers the delete, and the authz
+ tables (`resource_scope_index`, `authz_assignments`,
+ `authz_membership_edges`) cascade from `projects`.
+ - Clean up anything written outside the test's project (the platform
+ project, the catalog tables) with the test's own `t.Cleanup` and
+ `context.Background()`.
+ - A project created over HTTP goes through the cached handler's unwrapped
+ service; remove it with `Harness.CleanupProject`.
## Errors
- Services and storage return `domain.Error` values. Each carries a `Code`, and
- `domain.Error` implements `Is` by code, so it matches through any wrapping.
-
- Ask "is this that error?" with `errors.Is`:
-
- ```go
- if errors.Is(err, domain.ErrAuthAttemptNotFound()) { ... }
- ```
-
- Do not pull the error apart with `errors.As` and compare `e.Code` by hand for
- that. Reach for `errors.As` only when you need a field from the error, and for a
- code switch only where you map codes to statuses, as `errorResponse` does.
-
- An error nobody classified is a fault on our side, so it ends up a 500.
- `errorResponse` does this by default. Keep that property in any layer that
- turns a service error into something else.
+ Services and storage return `domain.Error` values, which match by `Code`
+ through any wrapping.
- So when a layer turns a service error into a refusal (401, 403 and the like),
- refuse only the errors that mean "no" and pass everything else on as internal:
+ - Ask "is this that error?" with `errors.Is`:
+ `errors.Is(err, domain.ErrAuthAttemptNotFound())`. Use `errors.As` only to
+ read a field, and switch on codes only where they map to statuses, as
+ `errorResponse` (`internal/api/error_handler.go`) does.
+ - An error nobody classified is a server fault and ends as a 5xx; keep that
+ in every layer that turns a service error into something else.
+ - A layer that turns errors into refusals (401, 403) refuses only the errors
+ that mean "no" and passes every server fault through, so it keeps its own
+ 5xx (`domain.IsServerFault`, as `errorResponse` reads it):
- ```go
- if errors.Is(err, domain.ErrTokenRevoked()) { // an answer that means "no"
- return ErrDenied
- }
- if errors.Is(err, domain.ErrInternal(nil)) {
- return err
- }
- return domain.ErrInternal(err) // we could not decide: the caller answers 500
- ```
+ ```go
+ if errors.Is(err, domain.ErrTokenRevoked()) { // an answer that means "no"
+ return ErrDenied
+ }
+ if domain.IsServerFault(err) { // 500, 501 or 503, decided by errorResponse
+ return err
+ }
+ return domain.ErrInternal(err) // anything else: we could not decide
+ ```
- Anything else means the layer could not decide, usually because a dependency
- such as the database failed, so the credential was never judged. Reporting that
- as a refusal tells clients their session is dead, and the SDKs clear their
- cookie on a 401, so an outage would sign every user out. `introspectionFailure`
- in `internal/api/security.go` follows this.
+ - A refusal for an error that means "we could not decide" (the database
+ failed, say) tells clients their session is dead, and the SDKs clear their
+ cookie on a 401: an outage would sign every user out.
## Format before push
- Before `git push` of Go changes under this tree, run
- `moon run server:format`.
-
- That task is check-only (`gofmt -l` on tracked `*.go`); it does not rewrite.
- On failure, run `gofmt -w` on the listed paths, then re-run
- `moon run server:format` until it passes. Do not push while it fails —
- `server:format` is part of `ci / full-pr`.
+ Run `moon run server:format` before pushing Go changes. It only checks
+ (`gofmt -l` on tracked `*.go`); on failure run `gofmt -w` on the listed paths
+ and re-run it. `server:format` is part of `ci / full-pr`.
## Redirect targets
- A URL the server sends a browser to, or stores to send one to later, comes from
- a caller until proven otherwise. Parse it with `httputil.ParseRedirectTarget` and
- nothing else: do not call `url.Parse` and check the pieces by hand, because a
- browser and a Go parser read the same string differently, and an open redirect
- lives in that gap (a backslash is a slash to a browser, `%2e` is a dot, a tab is
- dropped, and a path starting with `//` names another host once the scheme and
- host are stripped).
-
- `ParseRedirectTarget` decides whether the URL is safe to send a browser to. The
- caller still decides which hosts it accepts, for example by comparing the origin
- with the request's.
+ A URL the server sends a browser to, or stores to send one to later, comes
+ from a caller until proven otherwise.
- When a new way around the check turns up, add the input to
- `internal/httputil/testdata/redirect_targets.json` and fix the function. The
- API mock's port of the check (`packages/api-mock/src/redirect-target.ts`) reads
- the same file, so fix it there too. Do not add a second
- check at the call site.
+ - Parse it with `httputil.ParseRedirectTarget` and nothing else, never
+ `url.Parse` plus checks by hand: a browser and a Go parser read the same
+ string differently (a backslash is a slash, `%2e` a dot, a tab is dropped,
+ a leading `//` names another host), and an open redirect lives in that gap.
+ - The function decides whether the URL is safe for a browser; the caller
+ still decides which hosts it accepts (for example, the request's origin).
+ - A new way around the check goes into
+ `internal/httputil/testdata/redirect_targets.json`, and the fix into the
+ function and into the API mock's port (`packages/api-mock/src/redirect-target.ts`),
+ which reads the same file. Never add a second check at the call site.