diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000000..de4b9077c0 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,31 @@ +# Copilot Code Review ΓÇö Global Rules + +## Review Style +- Be concise and factual. +- Use short bullet points. +- No praise, no summaries, no speculation. +- Do not explain obvious language syntax. + +## Review Scope Guardrails +- Comment only on changed lines or directly affected code. +- If context is insufficient, say: "Insufficient context to assess." +- Do NOT infer intent or future design. + +## Comment Format +Use one of the following prefixes only: +- **[BLOCKER]** ΓÇö Correctness, safety, resource leak, or API stability issue. +- **[ISSUE]** ΓÇö Likely bug or deviation from established patterns. +- **[SUGGESTION]** ΓÇö Improvement that is not blocking. +- **[QUESTION]** ΓÇö Clarification needed from the author. + +## Output Constraints +- Max 2 comments per concern. +- Group related observations into a single comment. +- Prefer actionable phrasing: "Missing `Close()` call on X" not "Consider adding cleanup." + +## Prohibited Behavior +- Do not guess architectural intent. +- Do not recommend refactors without a concrete defect. +- Do not suggest changes unrelated to the diff. +- Do not attempt to re-derive CI results. +- If CI failures are visible in the PR, explain the failure concisely. diff --git a/.github/instructions/v2-shim.instructions.md b/.github/instructions/v2-shim.instructions.md new file mode 100644 index 0000000000..b65dfa5c70 --- /dev/null +++ b/.github/instructions/v2-shim.instructions.md @@ -0,0 +1,146 @@ +--- +applyTo: + - "cmd/containerd-shim-runhcs-v1/**/*.go" + - "cmd/containerd-shim-lcow-v2/**/*.go" + - "internal/hcsoci/**/*.go" + - "internal/uvm/**/*.go" + - "internal/layers/**/*.go" + - "internal/hcs/**/*.go" + - "internal/gcs/**/*.go" + - "internal/cow/**/*.go" + - "internal/resources/**/*.go" + - "internal/jobcontainers/**/*.go" + - "internal/guest/**/*.go" + - "internal/shim/**/*.go" + - "internal/controller/**/*.go" + - "internal/vm/**/*.go" +--- + +# V2 Shim — Code Review Rules + +Review rules for the containerd shim v2 path: shim binaries, HCS/OCI bridge, +UVM lifecycle, resource management, guest compute service, VM controller, +and container/process abstractions. + +The **primary lens** is Go conventions and best practices. The hcsshim-specific +rules below extend — never override — standard Go guidelines. + +--- + +## Go Conventions & Best Practices + +### Naming +- Follow [Effective Go](https://go.dev/doc/effective_go) naming: MixedCaps, no underscores in Go names. +- Package names are lowercase, single-word, no plurals. Avoid stutter (`hcs.HCSSystem` -> `hcs.System`). +- Interfaces named after the method when single-method (`io.Reader`, not `io.IReader`). +- Acronyms are all-caps (`ID`, `HTTP`, `UVM`), not `Id`, `Http`. + +### Exported vs Unexported +- **If an exported symbol has no callers outside its package, unexport it.** +- Flag new exports that are only used internally — keep the API surface minimal. +- Exported types, functions, and methods MUST have doc comments (`// TypeName ...`). +- Doc comments start with the symbol name and describe *what*, not *how*. + +### Error Handling +- Use `%w` for error wrapping with `fmt.Errorf`; flag bare `%v` on error values. +- Return errors, don't panic. Panics are only for truly unrecoverable programmer bugs. +- Check every returned error. Flag `_ = fn()` where `fn` returns an error (unless in cleanup with a log). +- Use `errors.Is` / `errors.As` for sentinel and type checks, never `==`. +- Prefer typed errors or sentinel errors over string matching. + +### Resource Management +- Every `io.Closer` must be `Close()`d — prefer `defer obj.Close()` immediately after creation. +- If `Close()` can fail and it matters, capture the error: `defer func() { retErr = obj.Close() }()`. +- Every handle (`os.File`, `syscall.Handle`, `windows.Handle`) must be closed in error paths. +- Flag resources created in a loop without per-iteration cleanup. + +### Concurrency +- Always pass `context.Context` as the first parameter; never store it in a struct. +- Goroutines must respect `ctx.Done()`. Flag goroutines without cancellation. +- Protect shared mutable state with `sync.Mutex` or channels. Flag unprotected access. +- Never capture loop variables in goroutine closures without rebinding (pre-Go 1.22). +- Prefer `sync.Once` for lazy initialization over manual bool + mutex patterns. + +### Interfaces +- Keep interfaces small (1-3 methods). Accept interfaces, return concrete types. +- Define interfaces at the consumer, not the implementer. +- Flag empty interfaces (`interface{}`/`any`) when a specific type would work. + +### Tests +- Test helpers must call `t.Helper()`. +- Use `t.Cleanup()` for teardown instead of manual defers when appropriate. +- Use table-driven tests for repetitive cases. +- Test names: `TestFunctionName_Condition_ExpectedResult`. +- Use `cmp.Diff` for struct comparison; `maps.Clone()` for copying maps. +- Use `functional` build tag for tests needing a live VM or container. + +### Packages & Imports +- Group imports: stdlib, external, internal. Use `goimports` ordering. +- No circular dependencies. Flag new cross-package imports that break layering. +- Internal packages should not leak implementation details through exported types. + +--- + +## hcsshim-Specific Rules + +### Resource Lifecycle (CRITICAL) + +- Every allocated resource MUST be registered with `r.Add(...)` or `r.SetLayers(...)`. +- Flag any `ResourceCloser` returned from a helper that is NOT added to the resource tracker. +- Flag resources allocated inside a loop where early `return` could skip `r.Add(...)`. +- `CreateContainer` and similar orchestrators MUST `defer` a call to + `resources.ReleaseResources(ctx, r, vm, true)` on error. +- `ResourceCloserList.Release()` releases in REVERSE order. Do not manually reorder. + +### HCS / UVM Object Lifecycle + +- Every `hcs.System` or `cow.Container` MUST be `Close()`d. +- Every `cow.Process` MUST be `Close()`d after `Wait()` completes. +- `uvm.CreateLCOW` / `uvm.CreateWCOW` returns a UVM that MUST be `Close()`d on error. +- `uvm.Start()` must be called after creation; flag orphaned UVMs. +- Flag SCSI, vSMB, vPMEM, or Plan9 mounts added to a UVM but not tracked for cleanup. + +### Memory & Handle Leaks + +- Flag `syscall.Handle` or `windows.Handle` values not closed in error paths. +- Flag goroutines that capture a `*cow.Process` or `*hcs.System` without ensuring + the object outlives the goroutine. + +### Concurrency (hcsshim-specific) + +- `hcs.System` operations are NOT goroutine-safe. Flag concurrent access without sync. +- UVM device maps (`scsiLocations`, `vsmb`, `plan9`, `vpmem`) are mutex-protected; + flag direct map access without holding the lock. + +### Package Layering + +Flag these violations: +- `internal/cow` importing anything above it (must be pure abstraction). +- `internal/hcs` importing from `internal/hcsoci` or `internal/uvm`. +- `internal/resources` importing from shim-level packages. +- `cmd/` directly importing `internal/hcs` instead of going through `internal/hcsoci`. +- `internal/controller/vm` importing from `internal/hcsoci`. + +### VM Controller & State Machine + +- Every state transition MUST be validated against allowed transitions. +- Flag direct state assignment that bypasses the transition validation function. +- Terminal states (`stopped`, `failed`) must not allow further transitions. +- `vmmanager.LifetimeManager` and `guestmanager.Manager` MUST handle idempotent `Stop()`/`Close()`. +- Flag implementations that panic or return untyped errors on double-close. + +### LCOW Shim V2 Service + +- Every containerd RPC handler MUST have proper context propagation and cancellation. +- Flag handlers that block indefinitely without respecting `ctx.Done()`. +- Flag missing cleanup in `Delete` — all resources from `Create` must be released. +- The shim MUST NOT leak UVM references across task boundaries. + +--- + +## Review Output + +- Max 2 comments per concern; group related items. +- Use **[BLOCKER]** only for resource leaks, correctness, safety, or API-breaking issues. +- Use **[ISSUE]** for likely bugs or pattern deviations. +- Use **[SUGGESTION]** for non-blocking improvements. diff --git a/.github/workflows/copilot-review.yml b/.github/workflows/copilot-review.yml new file mode 100644 index 0000000000..503d4b45ae --- /dev/null +++ b/.github/workflows/copilot-review.yml @@ -0,0 +1,82 @@ +# Auto-request GitHub Copilot review on PRs that touch v2 shim code. +# +# Copilot review is guidance-only (non-blocking). CI remains the enforcement gate. +# The path filter ensures Copilot is NOT invoked on unrelated PRs (docs, scripts, etc.). + +name: Copilot Review (V2 Shim) + +on: + pull_request: + paths: + # Shim binaries + - "cmd/containerd-shim-runhcs-v1/**" + - "cmd/containerd-shim-lcow-v2/**" + # Core shim internals + - "internal/hcsoci/**" + - "internal/uvm/**" + - "internal/layers/**" + - "internal/hcs/**" + - "internal/gcs/**" + - "internal/cow/**" + - "internal/resources/**" + - "internal/jobcontainers/**" + - "internal/guest/**" + - "internal/shim/**" + # VM controller & manager interfaces + - "internal/controller/**" + - "internal/vm/**" + +permissions: + contents: read + pull-requests: write + +jobs: + copilot-review: + if: github.event.pull_request.draft == false + runs-on: ubuntu-latest + steps: + - name: Request Copilot Review + uses: actions/github-script@v7 + with: + script: | + const pr = context.payload.pull_request; + + // Check if Copilot is already a reviewer to avoid duplicate requests. + const reviews = await github.rest.pulls.listRequestedReviewers({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + }); + const alreadyRequested = reviews.data.users.some( + (u) => u.login === 'copilot-pull-request-reviewer' + ); + if (alreadyRequested) { + core.info('Copilot review already requested ΓÇö skipping.'); + return; + } + + // Request Copilot as a reviewer. + try { + await github.rest.pulls.requestReviewers({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + reviewers: ['copilot-pull-request-reviewer'], + }); + core.info(`Copilot review requested on PR #${pr.number}`); + } catch (err) { + // Non-fatal: Copilot may not be enabled on the repo. + core.warning(`Could not request Copilot review: ${err.message}`); + } + + // Add a label to indicate Copilot review was requested. + try { + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pr.number, + labels: ['copilot-review'], + }); + } catch (err) { + core.info(`Could not add label: ${err.message}`); + }