Skip to content

Commit 54c2f19

Browse files
author
Shreyansh Sancheti
committed
Add Copilot code review for v2 shim PRs
Add automated GitHub Copilot review that triggers only when v2 shim code paths are modified. Copilot is guidance-only; CI remains the gate. - copilot-instructions.md: global review tone and format rules - v2-shim.instructions.md: Go best practices + hcsshim-specific rules (resource lifecycle, handle leaks, concurrency, package layering) - copilot-review.yml: workflow to auto-request Copilot review on PRs touching shim binaries, internal/hcs*, uvm, controller, vm packages Signed-off-by: Shreyansh Sancheti <shreyanshjain7174@gmail.com> Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
1 parent 5a0252a commit 54c2f19

3 files changed

Lines changed: 259 additions & 0 deletions

File tree

‎.github/copilot-instructions.md‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
# Copilot Code Review ΓÇö Global Rules
2+
3+
## Review Style
4+
- Be concise and factual.
5+
- Use short bullet points.
6+
- No praise, no summaries, no speculation.
7+
- Do not explain obvious language syntax.
8+
9+
## Review Scope Guardrails
10+
- Comment only on changed lines or directly affected code.
11+
- If context is insufficient, say: "Insufficient context to assess."
12+
- Do NOT infer intent or future design.
13+
14+
## Comment Format
15+
Use one of the following prefixes only:
16+
- **[BLOCKER]** ΓÇö Correctness, safety, resource leak, or API stability issue.
17+
- **[ISSUE]** ΓÇö Likely bug or deviation from established patterns.
18+
- **[SUGGESTION]** ΓÇö Improvement that is not blocking.
19+
- **[QUESTION]** ΓÇö Clarification needed from the author.
20+
21+
## Output Constraints
22+
- Max 2 comments per concern.
23+
- Group related observations into a single comment.
24+
- Prefer actionable phrasing: "Missing `Close()` call on X" not "Consider adding cleanup."
25+
26+
## Prohibited Behavior
27+
- Do not guess architectural intent.
28+
- Do not recommend refactors without a concrete defect.
29+
- Do not suggest changes unrelated to the diff.
30+
- Do not attempt to re-derive CI results.
31+
- If CI failures are visible in the PR, explain the failure concisely.

‎.github/instructions/v2-shim.instructions.md‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
---
2+
applyTo:
3+
- "cmd/containerd-shim-runhcs-v1/**/*.go"
4+
- "cmd/containerd-shim-lcow-v2/**/*.go"
5+
- "internal/hcsoci/**/*.go"
6+
- "internal/uvm/**/*.go"
7+
- "internal/layers/**/*.go"
8+
- "internal/hcs/**/*.go"
9+
- "internal/gcs/**/*.go"
10+
- "internal/cow/**/*.go"
11+
- "internal/resources/**/*.go"
12+
- "internal/jobcontainers/**/*.go"
13+
- "internal/guest/**/*.go"
14+
- "internal/shim/**/*.go"
15+
- "internal/controller/**/*.go"
16+
- "internal/vm/**/*.go"
17+
---
18+
19+
# V2 Shim — Code Review Rules
20+
21+
Review rules for the containerd shim v2 path: shim binaries, HCS/OCI bridge,
22+
UVM lifecycle, resource management, guest compute service, VM controller,
23+
and container/process abstractions.
24+
25+
The **primary lens** is Go conventions and best practices. The hcsshim-specific
26+
rules below extend — never override — standard Go guidelines.
27+
28+
---
29+
30+
## Go Conventions & Best Practices
31+
32+
### Naming
33+
- Follow [Effective Go](https://go.dev/doc/effective_go) naming: MixedCaps, no underscores in Go names.
34+
- Package names are lowercase, single-word, no plurals. Avoid stutter (`hcs.HCSSystem` -> `hcs.System`).
35+
- Interfaces named after the method when single-method (`io.Reader`, not `io.IReader`).
36+
- Acronyms are all-caps (`ID`, `HTTP`, `UVM`), not `Id`, `Http`.
37+
38+
### Exported vs Unexported
39+
- **If an exported symbol has no callers outside its package, unexport it.**
40+
- Flag new exports that are only used internally — keep the API surface minimal.
41+
- Exported types, functions, and methods MUST have doc comments (`// TypeName ...`).
42+
- Doc comments start with the symbol name and describe *what*, not *how*.
43+
44+
### Error Handling
45+
- Use `%w` for error wrapping with `fmt.Errorf`; flag bare `%v` on error values.
46+
- Return errors, don't panic. Panics are only for truly unrecoverable programmer bugs.
47+
- Check every returned error. Flag `_ = fn()` where `fn` returns an error (unless in cleanup with a log).
48+
- Use `errors.Is` / `errors.As` for sentinel and type checks, never `==`.
49+
- Prefer typed errors or sentinel errors over string matching.
50+
51+
### Resource Management
52+
- Every `io.Closer` must be `Close()`d — prefer `defer obj.Close()` immediately after creation.
53+
- If `Close()` can fail and it matters, capture the error: `defer func() { retErr = obj.Close() }()`.
54+
- Every handle (`os.File`, `syscall.Handle`, `windows.Handle`) must be closed in error paths.
55+
- Flag resources created in a loop without per-iteration cleanup.
56+
57+
### Concurrency
58+
- Always pass `context.Context` as the first parameter; never store it in a struct.
59+
- Goroutines must respect `ctx.Done()`. Flag goroutines without cancellation.
60+
- Protect shared mutable state with `sync.Mutex` or channels. Flag unprotected access.
61+
- Never capture loop variables in goroutine closures without rebinding (pre-Go 1.22).
62+
- Prefer `sync.Once` for lazy initialization over manual bool + mutex patterns.
63+
64+
### Interfaces
65+
- Keep interfaces small (1-3 methods). Accept interfaces, return concrete types.
66+
- Define interfaces at the consumer, not the implementer.
67+
- Flag empty interfaces (`interface{}`/`any`) when a specific type would work.
68+
69+
### Tests
70+
- Test helpers must call `t.Helper()`.
71+
- Use `t.Cleanup()` for teardown instead of manual defers when appropriate.
72+
- Use table-driven tests for repetitive cases.
73+
- Test names: `TestFunctionName_Condition_ExpectedResult`.
74+
- Use `cmp.Diff` for struct comparison; `maps.Clone()` for copying maps.
75+
- Use `functional` build tag for tests needing a live VM or container.
76+
77+
### Packages & Imports
78+
- Group imports: stdlib, external, internal. Use `goimports` ordering.
79+
- No circular dependencies. Flag new cross-package imports that break layering.
80+
- Internal packages should not leak implementation details through exported types.
81+
82+
---
83+
84+
## hcsshim-Specific Rules
85+
86+
### Resource Lifecycle (CRITICAL)
87+
88+
- Every allocated resource MUST be registered with `r.Add(...)` or `r.SetLayers(...)`.
89+
- Flag any `ResourceCloser` returned from a helper that is NOT added to the resource tracker.
90+
- Flag resources allocated inside a loop where early `return` could skip `r.Add(...)`.
91+
- `CreateContainer` and similar orchestrators MUST `defer` a call to
92+
`resources.ReleaseResources(ctx, r, vm, true)` on error.
93+
- `ResourceCloserList.Release()` releases in REVERSE order. Do not manually reorder.
94+
95+
### HCS / UVM Object Lifecycle
96+
97+
- Every `hcs.System` or `cow.Container` MUST be `Close()`d.
98+
- Every `cow.Process` MUST be `Close()`d after `Wait()` completes.
99+
- `uvm.CreateLCOW` / `uvm.CreateWCOW` returns a UVM that MUST be `Close()`d on error.
100+
- `uvm.Start()` must be called after creation; flag orphaned UVMs.
101+
- Flag SCSI, vSMB, vPMEM, or Plan9 mounts added to a UVM but not tracked for cleanup.
102+
103+
### Memory & Handle Leaks
104+
105+
- Flag `syscall.Handle` or `windows.Handle` values not closed in error paths.
106+
- Flag goroutines that capture a `*cow.Process` or `*hcs.System` without ensuring
107+
the object outlives the goroutine.
108+
109+
### Concurrency (hcsshim-specific)
110+
111+
- `hcs.System` operations are NOT goroutine-safe. Flag concurrent access without sync.
112+
- UVM device maps (`scsiLocations`, `vsmb`, `plan9`, `vpmem`) are mutex-protected;
113+
flag direct map access without holding the lock.
114+
115+
### Package Layering
116+
117+
Flag these violations:
118+
- `internal/cow` importing anything above it (must be pure abstraction).
119+
- `internal/hcs` importing from `internal/hcsoci` or `internal/uvm`.
120+
- `internal/resources` importing from shim-level packages.
121+
- `cmd/` directly importing `internal/hcs` instead of going through `internal/hcsoci`.
122+
- `internal/controller/vm` importing from `internal/hcsoci`.
123+
124+
### VM Controller & State Machine
125+
126+
- Every state transition MUST be validated against allowed transitions.
127+
- Flag direct state assignment that bypasses the transition validation function.
128+
- Terminal states (`stopped`, `failed`) must not allow further transitions.
129+
- `vmmanager.LifetimeManager` and `guestmanager.Manager` MUST handle idempotent `Stop()`/`Close()`.
130+
- Flag implementations that panic or return untyped errors on double-close.
131+
132+
### LCOW Shim V2 Service
133+
134+
- Every containerd RPC handler MUST have proper context propagation and cancellation.
135+
- Flag handlers that block indefinitely without respecting `ctx.Done()`.
136+
- Flag missing cleanup in `Delete` — all resources from `Create` must be released.
137+
- The shim MUST NOT leak UVM references across task boundaries.
138+
139+
---
140+
141+
## Review Output
142+
143+
- Max 2 comments per concern; group related items.
144+
- Use **[BLOCKER]** only for resource leaks, correctness, safety, or API-breaking issues.
145+
- Use **[ISSUE]** for likely bugs or pattern deviations.
146+
- Use **[SUGGESTION]** for non-blocking improvements.

‎.github/workflows/copilot-review.yml‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
# Auto-request GitHub Copilot review on PRs that touch v2 shim code.
2+
#
3+
# Copilot review is guidance-only (non-blocking). CI remains the enforcement gate.
4+
# The path filter ensures Copilot is NOT invoked on unrelated PRs (docs, scripts, etc.).
5+
6+
name: Copilot Review (V2 Shim)
7+
8+
on:
9+
pull_request:
10+
paths:
11+
# Shim binaries
12+
- "cmd/containerd-shim-runhcs-v1/**"
13+
- "cmd/containerd-shim-lcow-v2/**"
14+
# Core shim internals
15+
- "internal/hcsoci/**"
16+
- "internal/uvm/**"
17+
- "internal/layers/**"
18+
- "internal/hcs/**"
19+
- "internal/gcs/**"
20+
- "internal/cow/**"
21+
- "internal/resources/**"
22+
- "internal/jobcontainers/**"
23+
- "internal/guest/**"
24+
- "internal/shim/**"
25+
# VM controller & manager interfaces
26+
- "internal/controller/**"
27+
- "internal/vm/**"
28+
29+
permissions:
30+
contents: read
31+
pull-requests: write
32+
33+
jobs:
34+
copilot-review:
35+
if: github.event.pull_request.draft == false
36+
runs-on: ubuntu-latest
37+
steps:
38+
- name: Request Copilot Review
39+
uses: actions/github-script@v7
40+
with:
41+
script: |
42+
const pr = context.payload.pull_request;
43+
44+
// Check if Copilot is already a reviewer to avoid duplicate requests.
45+
const reviews = await github.rest.pulls.listRequestedReviewers({
46+
owner: context.repo.owner,
47+
repo: context.repo.repo,
48+
pull_number: pr.number,
49+
});
50+
const alreadyRequested = reviews.data.users.some(
51+
(u) => u.login === 'copilot-pull-request-reviewer'
52+
);
53+
if (alreadyRequested) {
54+
core.info('Copilot review already requested ΓÇö skipping.');
55+
return;
56+
}
57+
58+
// Request Copilot as a reviewer.
59+
try {
60+
await github.rest.pulls.requestReviewers({
61+
owner: context.repo.owner,
62+
repo: context.repo.repo,
63+
pull_number: pr.number,
64+
reviewers: ['copilot-pull-request-reviewer'],
65+
});
66+
core.info(`Copilot review requested on PR #${pr.number}`);
67+
} catch (err) {
68+
// Non-fatal: Copilot may not be enabled on the repo.
69+
core.warning(`Could not request Copilot review: ${err.message}`);
70+
}
71+
72+
// Add a label to indicate Copilot review was requested.
73+
try {
74+
await github.rest.issues.addLabels({
75+
owner: context.repo.owner,
76+
repo: context.repo.repo,
77+
issue_number: pr.number,
78+
labels: ['copilot-review'],
79+
});
80+
} catch (err) {
81+
core.info(`Could not add label: ${err.message}`);
82+
}

0 commit comments

Comments
 (0)