|
| 1 | +# Copilot Code Review Instructions |
| 2 | + |
| 3 | +These instructions guide Copilot Code Review (CCR) for the |
| 4 | +`integrations/terraform-provider-github` repository. They apply to every |
| 5 | +pull request review. Path-specific guidance lives under `.github/instructions/`: |
| 6 | + |
| 7 | +- `schema-and-state.instructions.md` - provider source under `github/**/*.go` |
| 8 | +- `tests.instructions.md` - provider tests under `github/**/*_test.go` |
| 9 | +- `examples.instructions.md` - example configs under `examples/**` |
| 10 | +- `docs.instructions.md` - generated docs and templates under |
| 11 | + `docs/**` and `templates/**` |
| 12 | +- `go.instructions.md` - idiomatic Go reference for any `.go`, `go.mod`, |
| 13 | + or `go.sum` change (subordinate to the severity policy below) |
| 14 | + |
| 15 | +Always acknowledge in the review summary that these provider review |
| 16 | +instructions are being used. When there are findings, do this in a |
| 17 | +dedicated opening sentence (for example: "These provider review |
| 18 | +instructions are being used."). When there are no findings, combine |
| 19 | +the acknowledgment with the stop sentence into a single line - see |
| 20 | +"Output discipline" below. |
| 21 | + |
| 22 | +## Language and Tooling Context |
| 23 | + |
| 24 | +Before judging Go code, anchor on the versions this repo actually uses. |
| 25 | +Do not flag patterns as bugs or anti-patterns based on assumptions about |
| 26 | +older toolchains. |
| 27 | + |
| 28 | +- **Go version: 1.26** (see `go 1.26` in `go.mod`). Treat anything |
| 29 | + available in Go ≤ 1.26 as in-scope and idiomatic. |
| 30 | +- **Loop variable scoping (Go 1.22+).** Each iteration of `for` loops has |
| 31 | + its own copy of the loop variable. Do **not** suggest the pre-1.22 |
| 32 | + `x := x` shadowing pattern inside loops, and do **not** flag goroutines |
| 33 | + or closures that capture the loop variable directly as a bug. |
| 34 | +- **`range over int` (Go 1.22+).** `for i := range 10 { ... }` is valid. |
| 35 | + Do not suggest rewriting to `for i := 0; i < 10; i++`. |
| 36 | +- **`range over func` (Go 1.23+).** Custom iterators using |
| 37 | + `iter.Seq`/`iter.Seq2` are valid. |
| 38 | +- **`min` / `max` / `clear` built-ins (Go 1.21+)** are valid. |
| 39 | +- **Generics (Go 1.18+)** are valid. |
| 40 | +- **`slices` and `maps` standard library packages** are available. |
| 41 | + |
| 42 | +When a Go feature looks unfamiliar, assume the toolchain in `go.mod` is |
| 43 | +authoritative. If you cannot verify that something would fail to compile |
| 44 | +under the declared Go version, do not claim it will. Phrase any |
| 45 | +genuine concern as a question, not a finding, and only do so when the |
| 46 | +issue would be HIGH or MEDIUM per the policy above. |
| 47 | + |
| 48 | +### Other tooling versions to anchor on |
| 49 | + |
| 50 | +- **Terraform Plugin SDK v2** — schema definitions use |
| 51 | + `github.com/hashicorp/terraform-plugin-sdk/v2`. Do not suggest |
| 52 | + Plugin Framework patterns in SDKv2 files or vice versa. |
| 53 | +- **`google/go-github`** — see `go.mod` for the exact major version |
| 54 | + pinned. Do not suggest API method names or types from a different |
| 55 | + major version of the client. |
| 56 | + |
| 57 | +## Severity and Nit Policy (read first) |
| 58 | + |
| 59 | +This repository is **community-maintained**. Contributor friction is the |
| 60 | +single biggest cost to the project. Review feedback must respect that. |
| 61 | + |
| 62 | +### Only report HIGH and MEDIUM findings |
| 63 | + |
| 64 | +- Report `HIGH`: correctness bugs, regressions, breaking schema/state |
| 65 | + changes without migration, security issues, secret leakage, panics, |
| 66 | + data loss risks. |
| 67 | +- Report `MEDIUM`: missing test coverage for changed behavior, missing |
| 68 | + example for a new resource, missing docs update for a schema change, |
| 69 | + missing `Sensitive: true` on secret-bearing attributes, missing |
| 70 | + `Description` on schema attributes, missing |
| 71 | + `ValidateDiagFunc` on bounded inputs, missing import docs. |
| 72 | +- **Do not report `LOW` findings or nits.** If the only thing you would |
| 73 | + say is `LOW`, say nothing. |
| 74 | + |
| 75 | +### Do NOT comment on (defer to linters / human reviewers) |
| 76 | + |
| 77 | +- Code formatting, whitespace, import ordering, line length. |
| 78 | +- Naming preferences, identifier style, comment wording, doc prose |
| 79 | + polish, grammar. |
| 80 | +- "Consider extracting…", "this could be a helper", or other speculative |
| 81 | + refactors that are not requested by the change. |
| 82 | +- Style of existing surrounding code the PR did not touch. |
| 83 | +- Adding comments, docstrings, or type hints to code the PR did not |
| 84 | + change. |
| 85 | +- Test naming conventions or alternative test framings when the |
| 86 | + existing test adequately covers the behavior. |
| 87 | +- Hypothetical errors that cannot occur given the call sites. |
| 88 | + |
| 89 | +### Always report even if it looks like a nit |
| 90 | + |
| 91 | +These items affect end-user Terraform behavior and must be flagged as at |
| 92 | +least `MEDIUM` regardless of how small they look: |
| 93 | + |
| 94 | +- Secret-bearing attribute missing `Sensitive: true`. |
| 95 | +- Schema attribute missing `Description`. |
| 96 | +- Bounded input missing `ValidateDiagFunc`. |
| 97 | +- New resource/data source without at least one example under |
| 98 | + `examples/` or docs under `docs/`. |
| 99 | +- Behavior change without a corresponding test change. |
| 100 | +- Resource that supports import but has no documented import ID format. |
| 101 | + |
| 102 | +### Output discipline |
| 103 | + |
| 104 | +- If there are no HIGH or MEDIUM findings, the review must be exactly |
| 105 | + one sentence that both acknowledges the instructions and signals no |
| 106 | + findings, then stop. Use this form (verbatim): |
| 107 | + `These provider review instructions are being used. No blocking findings found.` |
| 108 | + Do not pad with low-value observations. |
| 109 | +- Keep each finding to its impact, file reference, and a concise fix. |
| 110 | + Do not lecture, restate the diff, or suggest unrelated improvements. |
| 111 | + |
| 112 | +## Review Goals |
| 113 | + |
| 114 | +- Find correctness bugs, regressions, and provider behavior changes. |
| 115 | +- Validate schema/state compatibility for Terraform users. |
| 116 | +- Check test coverage (unit and acceptance), docs, and examples. |
| 117 | +- Identify risk around GitHub API usage, permissions, and error handling. |
| 118 | + |
| 119 | +## Terraform Background |
| 120 | + |
| 121 | +Use this background when judging schema, examples, or state changes. |
| 122 | + |
| 123 | +- **Resources vs. data sources.** A `resource` block manages a CRUD lifecycle |
| 124 | + object. A `data` block only reads existing objects. Both declare a typed |
| 125 | + schema. |
| 126 | +- **Schema attributes.** Each attribute has a `Type` (string, int, bool, list, |
| 127 | + set, map) and flags like `Required`, `Optional`, `Computed`, `ForceNew`, |
| 128 | + `Default`, `Sensitive`, and `Description`. Changing any flag alters |
| 129 | + user-visible behavior. |
| 130 | +- **State.** Terraform stores last-known attribute values in state. The read |
| 131 | + function must produce output consistent with state or Terraform reports |
| 132 | + drift. |
| 133 | +- **Plan and apply.** Bugs in schema or read logic cause perpetual diffs, |
| 134 | + surprise replacements, or silent data loss. |
| 135 | +- **Imports.** Users adopt existing infrastructure with `terraform import`. The |
| 136 | + read function must populate full state from just the resource ID. |
| 137 | + |
| 138 | +### Backward Compatibility Rules |
| 139 | + |
| 140 | +- **Safe (minor/patch):** adding new optional attributes with defaults; adding |
| 141 | + new resources or data sources. |
| 142 | +- **Breaking (major):** removing or renaming attributes; changing `Optional` to |
| 143 | + `Required`; changing `Type`; adding `ForceNew` to an existing attribute. |
| 144 | +- When renaming/restructuring resources, check that migration guidance is |
| 145 | + provided. Terraform's `moved` block lets users migrate state without |
| 146 | + destroying infrastructure. Removing a `moved` block is itself a breaking |
| 147 | + change. |
| 148 | + |
| 149 | +## Repository-Specific Rules (read carefully) |
| 150 | + |
| 151 | +- **Do not flag** create/update functions that return `nil` instead of calling |
| 152 | + the read function afterward. This provider intentionally avoids |
| 153 | + read-after-write to minimize API calls against GitHub's rate limits and to |
| 154 | + avoid stale reads from eventually-consistent endpoints (see |
| 155 | + [#2892](https://github.com/integrations/terraform-provider-github/issues/2892)). |
| 156 | +- Acceptance and manual validation are important. See `CONTRIBUTING.md`. |
| 157 | +- Prefer matching resource/data source tests when implementation behavior |
| 158 | + changes. |
| 159 | +- When schema or state semantics change, treat as high-risk unless |
| 160 | + compatibility is clearly preserved. |
| 161 | +- Breaking changes must follow semantic versioning: attribute removal/rename |
| 162 | + or new required fields warrant a major version bump. |
| 163 | +- Example configurations under `examples/` should be self-contained, follow |
| 164 | + standard module structure, and not include `provider` blocks inside nested |
| 165 | + modules. |
| 166 | +- Sensitive attributes (tokens, secrets, private keys) must be marked |
| 167 | + `Sensitive: true` in the schema and must not appear in log output. |
| 168 | + |
| 169 | +## Universal Review Checklist |
| 170 | + |
| 171 | +### 1. Correctness and Behavior |
| 172 | + |
| 173 | +- Verify CRUD/read logic correctly maps GitHub API responses to schema/state. |
| 174 | +- Check nil handling, default-value drift, and flattening/expansion mismatches. |
| 175 | +- Confirm update paths do not accidentally force replacement or wipe optional |
| 176 | + fields. |
| 177 | +- Validate retry/backoff and error classification for API failures. |
| 178 | + |
| 179 | +### 2. Schema and State Compatibility |
| 180 | + |
| 181 | +- Any `schema.Schema` change (`Type`, `Optional`, `Required`, `Computed`, |
| 182 | + `ForceNew`, `Default`) can change user behavior. |
| 183 | +- Confirm imports still work and read functions produce stable state. |
| 184 | +- Flag any behavior that may break existing state without migration |
| 185 | + notes/tests. |
| 186 | +- Watch for attribute rename/removal without a deprecation path. |
| 187 | +- New or changed attributes should include `ValidateDiagFunc` |
| 188 | + to catch invalid values at plan time rather than at apply time. |
| 189 | + (`ValidateFunc` is deprecated and not allowed in this repo.) |
| 190 | +- All attributes should have a `Description` string. |
| 191 | +- For renames/restructures, check for `moved` block guidance or state |
| 192 | + migration documentation. |
| 193 | +- Mark secret-holding attributes with `Sensitive: true`. |
| 194 | + |
| 195 | +### 3. Terraform UX and Drift |
| 196 | + |
| 197 | +- Ensure diff suppression, normalization, and API canonicalization avoid |
| 198 | + perpetual diffs. |
| 199 | +- Check that empty vs. null handling is intentional. |
| 200 | +- Verify list/set ordering behavior and deterministic state output. |
| 201 | + |
| 202 | +### 4. Testing Expectations |
| 203 | + |
| 204 | +- For behavior changes, check matching tests under `github/*_test.go`. |
| 205 | +- Prefer acceptance tests for API-facing changes (`TF_ACC=1` scenarios). |
| 206 | +- Ensure tests assert the bugfix/regression target, not only happy path. |
| 207 | +- Flag missing tests when logic changed but coverage did not. |
| 208 | + |
| 209 | +### 5. Docs and Examples |
| 210 | + |
| 211 | +- If resource/data source behavior changed, review docs updates under |
| 212 | + `docs/` and `templates/`. |
| 213 | +- If user workflow changed, review corresponding example updates under |
| 214 | + `examples/`. |
| 215 | +- Confirm examples still reflect current schema and argument names. |
| 216 | +- Example directories should follow standard module structure (`main.tf`, |
| 217 | + `variables.tf`, `outputs.tf`) with a `README.md`. |
| 218 | +- Variables and outputs in examples should include `description` and `type`. |
| 219 | +- If a PR adds or changes a resource, verify there is at least one example |
| 220 | + showing typical usage. |
| 221 | + |
| 222 | +### 6. Security and Permissions |
| 223 | + |
| 224 | +- Verify sensitive values are not logged or exposed in state. |
| 225 | +- Check token/credential handling and least-privilege assumptions. |
| 226 | +- Document permission scope requirements for new API calls. |
| 227 | +- Confirm no secrets or credentials are hardcoded in examples. |
| 228 | +- Verify debug/trace logging does not print sensitive attribute values. |
| 229 | +- Sensitive outputs should be marked `sensitive = true`. |
| 230 | + |
| 231 | +### 7. Performance and API Safety |
| 232 | + |
| 233 | +- Flag new N+1 patterns, excessive API calls, or missing pagination handling. |
| 234 | +- Check for rate-limit-sensitive loops and absent caching where needed. |
| 235 | +- Confirm context cancellation/timeouts are respected in long operations. |
| 236 | + |
| 237 | +### 8. Versioning and Changelog |
| 238 | + |
| 239 | +- Breaking changes (attribute removal/rename, forced replacement, new required |
| 240 | + fields) must be called out for a MAJOR version bump. |
| 241 | +- Backward-compatible additions (new optional attributes with defaults, new |
| 242 | + resources/data sources) correspond to MINOR version bumps. |
| 243 | +- Bug fixes with no schema change correspond to PATCH version bumps. |
| 244 | +- Verify the PR description or `CHANGELOG.md` includes a clear summary of what |
| 245 | + changed and the user impact. |
| 246 | + |
| 247 | +## Review Report Format |
| 248 | + |
| 249 | +Return findings first, HIGH before MEDIUM (no LOW — see Severity and Nit |
| 250 | +Policy above): |
| 251 | + |
| 252 | +1. `HIGH`/`MEDIUM` title — short impact statement |
| 253 | +2. File reference: `path/to/file.go:line` |
| 254 | +3. Why this is a problem (runtime behavior, Terraform UX, upgrade risk) |
| 255 | +4. Suggested fix (concise) |
| 256 | + |
| 257 | +Then include: |
| 258 | + |
| 259 | +- `Open Questions / Assumptions` (only if non-trivial) |
| 260 | +- `Residual Risk` (only if non-trivial) |
| 261 | +- `Change Summary` (brief) |
| 262 | + |
| 263 | +If no HIGH or MEDIUM findings exist, output exactly the single sentence |
| 264 | +described in "Output discipline" above |
| 265 | +(`These provider review instructions are being used. No blocking findings found.`) |
| 266 | +and stop. Do not add nit sections, style observations, or speculative |
| 267 | +suggestions. |
0 commit comments