diff --git a/.agents/skills/architecture-review/SKILL.md b/.agents/skills/architecture-review/SKILL.md new file mode 100644 index 0000000..2ba404c --- /dev/null +++ b/.agents/skills/architecture-review/SKILL.md @@ -0,0 +1,44 @@ +--- +name: architecture-review +description: Judge a change against the architecture as a whole, not the diff alone. Use on every review of a pull request alongside the correctness review. Checks spec divergence, contracts, error and write ownership, parity, structure, tests, and docs. +--- + +# Architecture review + +A diff can be correct line by line and still make the system worse: a second copy of a fact, a rule in the wrong layer, a patch over a symptom of a boundary that is wrong. Read the change, read the spec sections it touches, and judge the change against the whole. This adds to the correctness review; it does not replace it. + +Read the repo's AGENTS.md for its gates, layering rules, and spec location. This skill points; the spec decides. If the spec is not in the checkout, say so and do not guess its content. + +## Questions + +A "yes" is a finding. Cite the spec as `path:line` and the code as `file:line`. A finding without a citation is an opinion; label it as one. + +1. **Spec.** Does the change differ from the spec? That is a blocker unless the same PR updates the spec and records a dated decision. A rationale in the PR body alone does not count. +2. **Contracts and wire.** Do wire types, routes, and clients come from the one contract source? Is a type hand-written that is already generated? Is generated output regenerated in the same PR? Does a fake, stub, or golden differ from the real component in shape, status, error body, or order? +3. **Error ownership.** Is there one error envelope and one code table? Are errors passed through unchanged, not re-shaped or matched by string? +4. **Single writer.** Does each record, table, field, and route have one owner and one writer? Does the change add a second record, resolver, constant set, or implementation of a concept that has an owner? Search the tree for the existing one and name the owner. +5. **Workaround on a workaround.** Does the change add a retry, poll, cache, flag, or special case over a boundary problem? Name the boundary that should move. +6. **Parity.** When a system is replaced, does the new one keep the old behavior? A deliberate break needs a recorded decision. New code must not read or call anything the spec deletes. +7. **Structure.** + - Layering: does an import cross a gate, or point from a lower layer up to a higher one? Does the change widen an import rule without a spec line? Is policy in the transport, or does code branch on a provider or backend name? Is the interface declared in its consumer? + - Duplicate types: is a type or concept defined again in another package? + - Package purpose and size: does the package keep one purpose? Does a file pass about 1,000 lines or a function about 150 without a stated reason (unless the repo sets other limits)? + - Logic in `cmd/`: is policy in a main package instead of a testable one? + - Grab-bag names: packages `util`, `common`, `misc`, `helpers`, `api`, `types`; files `types.go`, `interfaces.go`. + - Go API rules: apply the `go-guide` skill to every new or changed exported API. +8. **Tests.** + - Each deleted test must cover only deleted code; a test deleted with live behavior still behind it is a regression. + - Behavior is pinned by a contract test through the public entry point, not by tests of internals. + - No sleeps; no history in tests or comments. +9. **Docs.** Docs ship with the code: a change to behavior, a package, or a route updates the spec or README in the same PR. A design-only PR is a finding. + +## Reporting + +- Lead with architecture findings, then correctness findings. +- Verify before you report: open the file, read the cited lines, and confirm the claim. Do not report from memory or from the diff alone. +- State each finding as the question number, the spec line, the `file:line`, and the smallest change that restores the boundary. +- Block on scope, unrecorded spec divergence, regression, a second source of truth, or a read of a deleted surface; block on a wrong layer only when it would be hard to move later. Mark the rest as non-blocking with their cost if left. +- A correct finding is not a requirement; weigh its cost against its benefit. +- Ignore pre-existing violations the change does not touch or worsen. Say so when the change touches one and leaves it as it was. +- Do not restate the spec or list what held. When there is nothing, say "no architecture findings" and name the spec sections read. +- Do not flag style. diff --git a/.agents/skills/go-guide/SKILL.md b/.agents/skills/go-guide/SKILL.md new file mode 100644 index 0000000..b96b34a --- /dev/null +++ b/.agents/skills/go-guide/SKILL.md @@ -0,0 +1,54 @@ +--- +name: go-guide +description: Go code and API rules to follow when writing or reviewing Go. Covers module and package layout, naming, API design, errors, concurrency, testing, and comments. Use before adding a package, an exported API, or tests. +--- + +# Go guide + +One rule per line, with its source where Go documents it. If the repo's AGENTS.md or CLAUDE.md differs, the repo wins. + +## Layout + +- Put commands in `cmd/` and private packages in `internal/`; keep `cmd/` to flags and wiring. [go.dev/doc/modules/layout](https://go.dev/doc/modules/layout) +- Give each package one purpose, named for what it provides. [go.dev/blog/package-names](https://go.dev/blog/package-names) +- Do not name a package `util`, `common`, `misc`, `helpers`, `api`, `types`, or `interfaces`. [go.dev/wiki/CodeReviewComments#package-names](https://go.dev/wiki/CodeReviewComments#package-names) +- Group files by concern, not by kind (no `types.go` or `interfaces.go` that collect everything). +- A large file or function needs a reason; split by concern before it grows. + +## Naming + +- Follow Effective Go and Code Review Comments for names, initialisms, and receivers. [go.dev/doc/effective_go#names](https://go.dev/doc/effective_go#names), [go.dev/wiki/CodeReviewComments](https://go.dev/wiki/CodeReviewComments) +- Do not stutter: `session.Store`, not `session.SessionStore`. [go.dev/blog/package-names](https://go.dev/blog/package-names) + +## API design + +- Take `context.Context` first and never store it in a struct. [pkg.go.dev/context](https://pkg.go.dev/context), [go.dev/wiki/CodeReviewComments#contexts](https://go.dev/wiki/CodeReviewComments#contexts) +- Return concrete types. Declare small interfaces in the consumer, not the producer, and not before a use exists. [go.dev/wiki/CodeReviewComments#interfaces](https://go.dev/wiki/CodeReviewComments#interfaces) +- Make the zero value useful, or provide a constructor that validates. [go.dev/doc/effective_go#allocation_new](https://go.dev/doc/effective_go#allocation_new) +- Configure with one options struct whose zero values are documented defaults. +- Document concurrency safety, ownership, and `Close` semantics (idempotent or not, what it waits for). [go.dev/doc/comment#type](https://go.dev/doc/comment#type) +- Treat errors as values: export sentinels or typed errors, match with `errors.Is` and `errors.As`, never by string. [go.dev/blog/go1.13-errors](https://go.dev/blog/go1.13-errors) +- Wrap with `%w` when the caller may match the cause; otherwise do not expose it. [go.dev/blog/go1.13-errors#whether-to-wrap](https://go.dev/blog/go1.13-errors#whether-to-wrap) +- Return errors; do not panic for normal error handling. [go.dev/wiki/CodeReviewComments#dont-panic](https://go.dev/wiki/CodeReviewComments#dont-panic) +- Return streams as `iter.Seq` or `iter.Seq2`. [pkg.go.dev/iter](https://pkg.go.dev/iter) +- Keep no global state: inject loggers (`*slog.Logger`) and environment readers. +- Report API breaks with apidiff before release. [pkg.go.dev/golang.org/x/exp/cmd/apidiff](https://pkg.go.dev/golang.org/x/exp/cmd/apidiff) + +## Concurrency + +- Share memory by communicating. [go.dev/doc/effective_go#sharing](https://go.dev/doc/effective_go#sharing) +- One goroutine owns each piece of data; others get copies or views. +- Every goroutine has an owner that starts it and a way to stop it. [go.dev/wiki/CodeReviewComments#goroutine-lifetimes](https://go.dev/wiki/CodeReviewComments#goroutine-lifetimes) +- Prefer synchronous functions; the caller adds concurrency. [go.dev/wiki/CodeReviewComments#synchronous-functions](https://go.dev/wiki/CodeReviewComments#synchronous-functions) + +## Testing + +- Write contract tests through the public entry point; test internals only for a stated reason (a race no contract test reaches, a codec with many edge cases). +- Use table rows for variants. [go.dev/wiki/TableDrivenTests](https://go.dev/wiki/TableDrivenTests) +- Use `testing/synctest` for time; do not sleep. [pkg.go.dev/testing/synctest](https://pkg.go.dev/testing/synctest) +- Write repo tooling in Go (`go run ./internal/` or a test), not shell scripts. + +## Comments + +- Write no history in comments (no PR numbers, dates, or "changed from"); commit messages hold history. +- Say what the code does and why it must; do not restate the code. [go.dev/doc/comment](https://go.dev/doc/comment) diff --git a/.claude/skills/verify/SKILL.md b/.agents/skills/verify/SKILL.md similarity index 100% rename from .claude/skills/verify/SKILL.md rename to .agents/skills/verify/SKILL.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..34424f6 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,9 @@ +# Copilot review instructions + +Review each pull request against the architecture as a whole, not the diff alone. + +- Read the installed `architecture-review` skill (`.agents/skills/architecture-review/SKILL.md`) first, then the gatekeeper checks in `.github/skills/gatekeeper-architecture-review/SKILL.md`, and answer their questions for every change. +- Apply the installed `go-guide` skill (`.agents/skills/go-guide/SKILL.md`) to every new or changed Go package, exported API, and test. +- Read the root `AGENTS.md` and the doc under `docs/` for the area a change touches. Cite it as `path:line` for each architecture finding. +- A credential value in a log, error, span, or temp file is a High-confidence finding. So is a path that skips proxy authentication or widens host matching. +- Do not flag pure style or formatting. Do not flag a violation that already existed and that the change does not touch. diff --git a/.github/skills/gatekeeper-architecture-review/SKILL.md b/.github/skills/gatekeeper-architecture-review/SKILL.md new file mode 100644 index 0000000..3f63b58 --- /dev/null +++ b/.github/skills/gatekeeper-architecture-review/SKILL.md @@ -0,0 +1,38 @@ +--- +name: gatekeeper-architecture-review +description: Gatekeeper-specific checks for the shared architecture-review skill, covering proxy authentication, credential injection, the MITM CA, token exchange, and the library boundary. Apply to every pull request, alongside the correctness review. +--- + +# Gatekeeper architecture checks + +Apply the installed shared skills first: `.agents/skills/architecture-review/SKILL.md` for the review method and questions, and `.agents/skills/go-guide/SKILL.md` for Go layout, API, and test rules. This file adds only what is specific to gatekeeper. If this file and the docs disagree, the docs win and this file is the bug. + +## Where the architecture is written + +Gatekeeper has no single spec file. Read the section that the change touches: + +- Root `AGENTS.md`: `Architecture`, `Key Types`, `How Credential Injection Works`, `Security Considerations`. +- `docs/content/concepts/`: TLS interception, credential injection, credential sources, network policy, MCP relay, observability, host gateway, Postgres data plane. +- `docs/content/reference/02-config-file.md`: the config contract, including `proxy.auth_token`. +- `docs/token-exchange-endpoint.md`: the contract with the STS that gatekeeper calls. + +If a file is not in your checkout, say so. Do not guess its content. Cite the doc as `path:line`. + +## Gatekeeper questions + +A "yes" is a finding. + +1. **Credential exposure.** Can a credential value reach a log line, span attribute, metric label, error returned to a client, response body, or temp file? Logs name the host and grant only. +2. **Proxy authentication.** On the HTTP paths (CONNECT, plain HTTP, `/relay/`), can a password that does not constant-time-equal a configured `auth_token` reach a resolver or the STS before the `407`? Does a `407` for a wrong password differ in body from one for no credentials? On the Postgres listener, does the run-token compare in `PostgresServer.authenticate` stop being constant-time, or fail with anything other than a Postgres `ErrorResponse`? +3. **Injection scope.** Does a credential match a host by anything other than exact name or an explicit glob? Is the host compared before port stripping or case folding? Can a client-supplied header replace or keep an injected credential? +4. **MITM CA.** Does the change write the CA private key anywhere other than the `0600` `ca.key` that `proxy.NewCA` creates, log it, or widen its file mode? Can a malformed CONNECT bypass interception? +5. **Token exchange.** Can the subject come from anything other than the configured source (`subject_from` or `subject_header`), such as a header an untrusted caller sets under proxy-auth mode? Is the actor token forwarded when `actor_token_from` is unset? Does a bot or empty subject still call the STS? Does a wire field differ from `docs/token-exchange-endpoint.md`? +6. **Source ownership.** Does a credential backend bypass `credentialsource.CredentialSource` or `RefreshingSource`? Does the proxy core branch on a backend type? Do identical `source` blocks now fetch more than once or start a second refresh goroutine? +7. **Library boundary.** Does the change add an `internal/` package, or make `proxy/` know about moat? Gatekeeper is an importable module. A public API or YAML field change is a contract change: it needs a doc update and a `CHANGELOG.md` entry that names the behavior change. +8. **Observability stays additive.** Does `proxy/proxy.go` import OTel? Instrumentation attaches through callbacks and `proxy/otel.go`, and must not change request behavior or errors. +9. **Fail-open and fail-closed.** Does a new error path open a connection that policy should block? PROXY protocol parsing stays fail-open on headerless connections only. Network policy and auth stay fail-closed. +10. **Tests.** Does a bug fix lack a regression test that failed first and names the real values from the report? Does a security change test only the accepted input and not the rejected one? + +## Reporting + +Follow the `Reporting` section of the shared `architecture-review` skill. Block on questions 1 to 5 whenever the answer is yes. diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 787f3a0..b48a908 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -38,7 +38,12 @@ jobs: prompt: | Use the /code-review skill to review this pull request. - This is a standalone credential-injecting TLS proxy. Review with these priorities: + This is a standalone credential-injecting TLS proxy. Read the root + AGENTS.md first. Then read .agents/skills/architecture-review/SKILL.md + and .github/skills/gatekeeper-architecture-review/SKILL.md and apply + them, and .agents/skills/go-guide/SKILL.md for Go code: judge the + change against the architecture as a whole, not the diff alone. + Review with these priorities: - Security issues (credential leaks, injection, TLS configuration, TOCTOU) - Race conditions, deadlocks, missing synchronization - Bugs, logic errors, nil/zero-value pitfalls diff --git a/AGENTS.md b/AGENTS.md index 9da2c34..7319001 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,6 +122,7 @@ Always work test-first. Write the test, run it and confirm it fails, then write ## Code Style - Follow standard Go conventions and `go fmt` formatting +- Follow the installed `.agents/skills/go-guide/SKILL.md` for Go layout, API design, concurrency, tests, and comments. Where it differs from this file, this file wins - Use `go vet` to catch common issues - No `internal/` packages — this is a library module meant to be imported @@ -132,6 +133,10 @@ Always work test-first. Write the test, run it and confirm it fails, then write - Scope is optional but encouraged (e.g., `feat(proxy): add header injection`) - Do not include `Co-Authored-By` lines for AI agents in commit messages +## Architecture Review + +Judge every change against the architecture as a whole, not the diff alone. Apply the installed `.agents/skills/architecture-review/SKILL.md`, then `.github/skills/gatekeeper-architecture-review/SKILL.md`. + ## Security Considerations This proxy handles sensitive credentials. When making changes: diff --git a/skills-lock.json b/skills-lock.json new file mode 100644 index 0000000..d463c4a --- /dev/null +++ b/skills-lock.json @@ -0,0 +1,17 @@ +{ + "version": 1, + "skills": { + "architecture-review": { + "source": "andybons/kit", + "sourceType": "github", + "skillPath": "skills/architecture-review/SKILL.md", + "computedHash": "c2f08c33b872d89039f2e577e48741ff3494ca7b698c6d55f54a0e020772625a" + }, + "go-guide": { + "source": "andybons/kit", + "sourceType": "github", + "skillPath": "skills/go-guide/SKILL.md", + "computedHash": "1888085fa84fa0d11580ac5d77252b997b7fb64d920e947bcb62f261f76dd678" + } + } +}