mirror of
https://github.com/microsoft/regorus.git
synced 2026-08-05 02:16:11 +00:00
feat(copilot): add multi-agent code review skills (#707)
Add Copilot review skills, project instructions, and coding agent setup for automated code review on regorus PRs. Files added: - .github/copilot-instructions.md — project context (no_std, 9 bindings, dual execution paths, deny lints, security-critical evaluation) - .github/skills/code-review/SKILL.md — fast single-agent review (~2 min) - .github/skills/deep-review/SKILL.md — multi-agent deep review (~12 min) - .github/copilot-setup-steps.yml — minimal coding agent environment Development and testing methodology: The skills were developed iteratively (v3 through v11.4) against a 460-line SARIF output module on the feature/sarif-output branch, which served as a controlled test bed with 25 known issues of varying severity (correctness, safety, API design, platform, security, performance). Each version was tested by running the skill via the Copilot CLI, then mapping discovered findings against the ground truth set to measure recall and precision. Key iterations: - v3: baseline single-agent (8/25 recall, 32%) - v7: 3 parallel agents + verification (14/25, 56%) - v10c: model diversity + adversarial pass (10/25, 40%) - v11.3: merged adversarial-verifier architecture (12/25 + 2 novel, 0 noise) - v11.4: domain expertise prompting (12/25 + 2 novel, 0 noise, full report) The final architecture uses 3 parallel discovery agents (with cross-model diversity and context asymmetry), risk-triggered micro-passes, and a single adversarial verifier that both validates candidates via disproval and hunts blind spots. Agents are prompted to reason from policy-author perspective across Rego/OPA, Azure Policy, and RVM workloads. Combined CR+DR catches 16-17/25 ground truth with zero false positives and produces verified findings with confidence levels, test gap analysis, and agent performance metrics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
parent
87f22a79ca
commit
88c7ef8228
@@ -0,0 +1,198 @@
|
||||
---
|
||||
name: code-review
|
||||
description: >-
|
||||
Fast multi-perspective code review for regorus. Use for everyday code reviews.
|
||||
Reviews from 3 perspectives with calibrated severity and noise filtering.
|
||||
allowed-tools: shell
|
||||
---
|
||||
|
||||
# Code Review Skill
|
||||
|
||||
## What You're Protecting
|
||||
|
||||
A bug in regorus can mean `allow` when the answer should be `deny`.
|
||||
Review this diff to find bugs that matter at that severity level.
|
||||
|
||||
Key constraints (details in copilot-instructions.md):
|
||||
- **Undefined ≠ false** — silent wrong policy results
|
||||
- **Panics across FFI** → permanent engine poisoning (process-wide)
|
||||
- **9 binding targets** → any API change has 9x blast radius
|
||||
- **Dual execution paths** — interpreter and RVM must agree
|
||||
- **`enforce_limit()`** required in accumulation loops
|
||||
|
||||
**Do not** run cargo, clippy, tests, or build commands. Diff-review only.
|
||||
|
||||
## Step 1: Get the Diff
|
||||
|
||||
```bash
|
||||
BASE=$(git merge-base upstream/main HEAD 2>/dev/null \
|
||||
|| git merge-base origin/main HEAD 2>/dev/null)
|
||||
if [ -z "$BASE" ]; then
|
||||
echo "ERROR: Cannot find upstream/main or origin/main. Cannot determine review scope."
|
||||
exit 1
|
||||
fi
|
||||
echo "Reviewing changes since: $BASE"
|
||||
git diff "$BASE"..HEAD --stat
|
||||
git diff "$BASE"..HEAD -- '*.rs' '*.toml' 'examples/'
|
||||
```
|
||||
|
||||
If the diff is empty, stop and report: "No changes found to review."
|
||||
|
||||
## Step 2: Triage and Inventory
|
||||
|
||||
Classify the diff before reviewing:
|
||||
- **Trivial/mechanical**: renames, formatting, comments, dep version bumps, generated code
|
||||
→ Report "No material issues found" unless something catches your eye. Skip Step 3.
|
||||
- **Targeted change**: ≤300 changed lines in a focused area → Review with relevant perspectives.
|
||||
- **Large/cross-cutting**: >300 lines or multiple subsystems → Review all perspectives.
|
||||
|
||||
**Quick inventory:** List every changed function/struct/pub item (one line each).
|
||||
At the end of Step 3, confirm you examined each one.
|
||||
|
||||
## Step 3: Review — Three Passes
|
||||
|
||||
**Your goal is breadth.** Cover the entire diff, don't fixate on one area.
|
||||
Report anything suspicious even if you're only 60% sure — better to include a
|
||||
Low finding than miss a Medium.
|
||||
|
||||
### Pass 1: Line-by-line correctness
|
||||
|
||||
Walk through every changed line. For each, ask:
|
||||
- What was the author's intent? Does the code achieve it for ALL inputs?
|
||||
- What happens with: empty, null, zero, max-size, wrong-type, nested, Undefined?
|
||||
- What happens on Windows? With non-ASCII? With empty string vs absent?
|
||||
- If output must follow a standard (SARIF, URI, JSON Schema): are all MUST
|
||||
requirements met? Reserved chars escaped? Required fields present?
|
||||
- What does the most common real-world input to this function look like?
|
||||
Does the code handle that correctly? What about the second and third most
|
||||
common patterns?
|
||||
|
||||
For suspicious code paths, trace a concrete value through them:
|
||||
```
|
||||
input = <concrete example>
|
||||
→ after line N: variable = <concrete value>
|
||||
→ after line M: result = <concrete value>
|
||||
→ expected: <what it should be>
|
||||
```
|
||||
Concrete traces strengthen Critical/High findings but are NOT required to
|
||||
report a finding. If something looks wrong, report it — even at Medium/Low
|
||||
confidence.
|
||||
|
||||
Use `view` to read surrounding context for anything suspicious.
|
||||
|
||||
### Pass 2: System-level consequences
|
||||
|
||||
Step back from individual lines:
|
||||
- Does this new API freeze anything via semver? (pub fields, pub types, pub mods
|
||||
without feature gates)
|
||||
- Could a caller misuse this API in a way the author didn't anticipate?
|
||||
- Resource consumption: is anything proportional to untrusted input without bounds?
|
||||
- Error handling: are errors propagated or silently swallowed? Appropriate types?
|
||||
- Does this interact badly with existing features? (feature flags, no_std, `arc`,
|
||||
dual interpreter/RVM paths)
|
||||
- If touching `src/engine.rs`, `src/lib.rs`, or `bindings/`: do all 9 targets handle it?
|
||||
- If touching `Cargo.toml` or `#[cfg(feature)]`: feature gate correctness, no_std?
|
||||
|
||||
### Pass 3: What's missing
|
||||
|
||||
Scan the diff stat one final time:
|
||||
- Are there files or functions you haven't examined closely? Look now.
|
||||
- For each new public function: what happens with every `Value` variant?
|
||||
(Null, Bool, Number, String, Array, Set, Object, Undefined)
|
||||
- What test cases would you write? Are the obvious ones present?
|
||||
- What does the code assume about inputs that isn't validated?
|
||||
- If control flow uses `break` in nested loops — does it exit the right level?
|
||||
|
||||
### Edge-Case Exploration
|
||||
|
||||
For each significant new function or data transformation:
|
||||
|
||||
1. **Boundary inputs**: empty collections, zero/max integers, single vs many,
|
||||
deeply nested
|
||||
2. **Type mismatches**: expected object with fields → gets string/array/Undefined?
|
||||
Silent default? Error? Wrong output passed downstream?
|
||||
3. **Platform variance**: Unix assumptions? (path separators, encoding, locale).
|
||||
Wrong output on Windows?
|
||||
4. **Composition**: How does this interact with other modules? Could a valid
|
||||
combination produce unexpected behavior?
|
||||
5. **Specification conformance**: If output follows a standard, are all MUST/SHOULD
|
||||
met? Reserved chars escaped? Required fields always present?
|
||||
|
||||
Only report edge cases with concrete example input → wrong output.
|
||||
|
||||
## Step 4: Design Considerations
|
||||
|
||||
Skip if the diff is trivial/mechanical or <50 changed lines.
|
||||
|
||||
Otherwise, briefly assess (2-3 sentences each, only if relevant):
|
||||
- Is there a fundamentally simpler way to achieve the same goal?
|
||||
- Does this duplicate existing infrastructure that could be reused?
|
||||
- Are there tradeoffs the author may not have considered?
|
||||
|
||||
Only suggest alternatives you can concretely describe with clear benefit.
|
||||
|
||||
## Step 5: Report
|
||||
|
||||
### Findings (sorted by severity)
|
||||
|
||||
For each finding:
|
||||
- **Severity**: Critical / High / Medium / Low
|
||||
- **Confidence**: High / Medium / Low
|
||||
- **Perspective**: which perspective found it
|
||||
- **Location**: file:line
|
||||
- **Issue**: one-sentence summary
|
||||
- **Trace**: concrete input → concrete intermediate values → concrete wrong output
|
||||
(strengthens Critical/High but not required for Medium/Low)
|
||||
- **Evidence**: the specific code (max 5 lines) and why it's wrong
|
||||
- **Suggestion**: concrete fix (include code snippet when possible)
|
||||
|
||||
**Confidence guide:**
|
||||
- **High**: you have a concrete trace showing wrong output
|
||||
- **Medium**: pattern match + plausible scenario but no full trace
|
||||
- **Low**: suspicious but cannot fully demonstrate the issue
|
||||
|
||||
**Severity calibration — lean toward reporting, not filtering.**
|
||||
A separate review step can always downgrade. If you're unsure between two
|
||||
severity levels, pick the higher one.
|
||||
|
||||
- **Critical**: Wrong policy result (allow/deny), panic reachable from FFI, security bypass.
|
||||
Every Critical MUST include: who triggers it, what specific input, why guards fail.
|
||||
If you can't construct a trigger path, downgrade to High.
|
||||
- **High**: Panic in non-FFI path, unbounded resource usage, API break, data loss/corruption
|
||||
- **Medium**: Logic error with limited blast radius, silent wrong output for edge-case inputs,
|
||||
missing bound on trusted path, design issue with concrete consequence
|
||||
- **Low**: Minor inefficiency with measurable impact, missing validation, documentation gap
|
||||
|
||||
**Do NOT report:**
|
||||
- Style preferences (naming, formatting) with no functional impact
|
||||
- Anything the compiler or ~53 deny lints would catch
|
||||
- "Consider using X" without explaining what goes wrong if you don't
|
||||
|
||||
**0 findings is valid** — do not manufacture findings without evidence.
|
||||
|
||||
**Calibration examples:**
|
||||
|
||||
Good finding:
|
||||
> HIGH | src/eval.rs:42 | `items[idx]` where `idx` comes from untrusted input
|
||||
> via `parse_array()` at line 38. No bounds check between parse and use.
|
||||
> **Fix:** `items.get(idx).ok_or_else(|| anyhow!("index out of bounds"))?`
|
||||
|
||||
Bad finding (reject):
|
||||
> "This unwrap could panic" — without verifying the value isn't guaranteed
|
||||
> `Some` by construction. Check first.
|
||||
|
||||
Bad finding (reject):
|
||||
> "Consider using a more descriptive variable name."
|
||||
|
||||
### Design Notes
|
||||
|
||||
Observations from Step 4 (if applicable).
|
||||
|
||||
### Coverage Check
|
||||
|
||||
Confirm: every function/struct from your inventory was examined in at least
|
||||
one pass. If any were skipped, note them and briefly assess.
|
||||
|
||||
### Summary
|
||||
|
||||
X findings (N critical, N high, N medium, N low). One sentence overall assessment.
|
||||
Reference in New Issue
Block a user