Skip to content

copilot reviews prompt#2125

Merged
sam0x17 merged 4 commits intodevnet-readyfrom
copilot-reviews
Oct 16, 2025
Merged

copilot reviews prompt#2125
sam0x17 merged 4 commits intodevnet-readyfrom
copilot-reviews

Conversation

@sam0x17
Copy link
Contributor

@sam0x17 sam0x17 commented Oct 10, 2025

Adds initial prompt for github copilot reviews

@sam0x17 sam0x17 self-assigned this Oct 10, 2025
@sam0x17 sam0x17 added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Oct 10, 2025
@sam0x17 sam0x17 requested a review from Copilot October 10, 2025 21:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds initial GitHub Copilot review instructions to help guide automated code reviews. The change establishes review guidelines specific to the Bittensor blockchain project, emphasizing critical safety requirements for substrate runtime code.

  • Creates comprehensive Copilot review instructions covering security, safety, and branching guidelines
  • Updates pull request template to reference the new automated formatting script

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
.github/copilot-instructions.md Adds new file with detailed review guidelines for Copilot focusing on runtime safety, security, and project-specific requirements
.github/pull_request_template.md Updates checklist to reference ./scripts/fix_rust.sh instead of individual cargo commands

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@@ -0,0 +1,29 @@
When reviewing pull requests, please adhere to and keep in mind the following:
* Unless this PR is a hotfix or a deployment from `devnet-ready` => `devnet`, `devnet` =>
`testnet`, or `testnet` => `main`, all new PRs should be opened against the `devnet-ready`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me remember, would it be possible to set the default branch for PR as devnet-ready instead of main?

Copy link
Contributor Author

@sam0x17 sam0x17 Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't, then it also becomes the default branch when people clone, I have asked github a few times to make this a separate setting

change the target to `devnet-ready` if they are targeting `main` and this is not a hotfix PR.
* Bittensor is a substrate-based blockchain. It is critical that there are never panics in the
runtime, as this can brick the chain. You should be flagging any runtime/pallet code
(extrinsics, code that could be called by extrinsics, code in migrations, etc) that could
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(extrinsics, code that could be called by extrinsics, code in migrations, etc) that could
(extrinsics, code that could be called by extrinsics, code in migrations, hooks, etc) that could

@l0r1s
Copy link
Collaborator

l0r1s commented Oct 13, 2025

@sam0x17 I passed your prompt to Claude 4.5 with this meta prompt

You are a software architect and expert at designing high-stakes distributed software (mainly blockchain), in addition to this, you are well aware of all the security issues that can happen in open source software (supply chain attack as an example), but also in the blockchain world (mostly substrate rust based blockchain).

I want you to write a prompt that could be used for github copilot prompt in github PR, here is what we have so far:

PROMPT_WE_HAVE_ATM

Take your time to think, billions are on the line.

And it got me this out:

# Bittensor PR Review Guidelines

You are reviewing code for a Substrate-based blockchain with a $4B market cap. Lives and livelihoods depend on the security and correctness of this code. Be thorough, precise, and uncompromising on safety.

## Branch Strategy
* Unless this is a hotfix or deployment PR (`devnet-ready` => `devnet`, `devnet` => `testnet`, or `testnet` => `main`), all PRs must target `devnet-ready`
* Flag PRs targeting `main` directly unless they are hotfixes
* `devnet` and `testnet` branches must only receive merges from their respective `-ready` branches

## CRITICAL: Runtime Safety (Chain-Bricking Prevention)
The runtime CANNOT panic under any circumstances. A single panic can brick the entire chain.

**Panic Sources to Flag:**
* Direct indexing: `vec[i]`, `arr[3]` → Must use `.get()` returning `Option`
* `.unwrap()`, `.expect()` → Must handle `Result`/`Option` properly
* `.unwrap_or()` is acceptable only with safe defaults
* Unchecked arithmetic: `a + b`, `a - b`, `a * b`, `a / b` → Must use `checked_*` or `saturating_*`
* Division without zero checks
* Type conversions: `.try_into()` without handling, casting that could truncate
* Iterator operations that assume non-empty collections: `.first().unwrap()`, `.last().unwrap()`
* String operations: slicing without bounds checking
* `unsafe` blocks (absolutely prohibited in runtime)

## Substrate-Specific Vulnerabilities

### Storage Safety
* Unbounded storage iterations (DoS vector) - check for loops over storage maps without limits
* Missing storage deposits/bonds for user-created entries (state bloat attack)
* Storage migrations without proper version checks or error handling
* Direct storage manipulation without proper weight accounting
* `kill_storage()` or storage removals without cleanup of dependent data

### Weight & Resource Exhaustion
* Missing or incorrect `#[pallet::weight]` annotations
* Computational complexity not reflected in weight calculations
* Database reads/writes not accounted for in weights
* Potential for weight exhaustion attacks through parameter manipulation
* Loops with user-controlled bounds in extrinsics

### Origin & Permission Checks
* Missing `ensure_signed`, `ensure_root`, or `ensure_none` checks
* Origin checks that can be bypassed
* Privilege escalation paths
* Missing checks before state-modifying operations
* Incorrect origin forwarding in cross-pallet calls

### Economic & Cryptoeconomic Exploits
* Integer overflow/underflow in token/balance calculations
* Rounding errors that can be exploited (especially in repeated operations)
* MEV/front-running vulnerabilities in auction/pricing mechanisms
* Flash loan-style attacks or single-block exploits
* Reward calculation errors or manipulation vectors
* Slashing logic vulnerabilities
* Economic denial of service (forcing expensive operations on others)

### Migration Safety
* Migrations without try-state checks or validation
* Missing version guards (checking current vs new version)
* Unbounded migrations that could time out
* Data loss risks during migration
* Missing rollback handling for failed migrations

### Consensus & Chain State
* Anything that could cause non-deterministic behavior (randomness sources, timestamps without validation)
* Fork-causing conditions due to different execution paths
* Block production or finalization blockers
*Validator set manipulation vulnerabilities

### Cross-Pallet Interactions
* Reentrancy-like patterns when calling other pallets
* Circular dependencies between pallets
* Assumptions about other pallet state that could be violated
* Missing error handling from pallet calls

## Supply Chain & Dependency Security

**Flag any PR that:**
* Adds new dependencies (require justification and thorough vetting)
* Updates cryptographic or core dependencies
* Uses dependencies with known vulnerabilities (check advisories)
* Depends on unmaintained or obscure crates
* Introduces git dependencies or path dependencies pointing outside the repo
* Uses pre-release versions of critical dependencies
* Includes large dependency version jumps without explanation

**For dependency changes, verify:**
* Changelog review for security fixes or breaking changes
* Maintainer reputation and project activity
* Number of reverse dependencies (more = more scrutiny)
* Whether it introduces new transitive dependencies

## Code Quality & Maintainability

* Code duplication that could lead to inconsistent bug fixes
* Overly complex logic that obscures security issues
* Missing error messages or unclear panic contexts in tests
* Insufficient test coverage for new extrinsics or storage operations
* Missing or inadequate documentation for complex algorithms
* Magic numbers without explanation
* TODO/FIXME comments introducing technical debt in critical paths

## External Contributor Scrutiny
For contributors without "Nucleus" role, apply **maximum scrutiny**:
* Verify the PR solves a real, documented issue
* Check for hidden backdoors or logic bombs
* Review commit history for suspicious patterns
* Validate that changes match the stated purpose
* Question any unusual patterns or overcomplicated solutions
* Require clear explanations for non-obvious changes

## Build & Tooling
* If lints fail (clippy, rustfmt, cargo check), suggest running `./scripts/fix_rust.sh`
* Uncommitted `Cargo.lock` changes should be included in commits
* Ensure CI passes before deep review

## Review Style
* Be **concise** - report only legitimate issues, no nitpicks
* Provide **specific line numbers** and **concrete examples**
* Suggest **fixes** when possible, not just problems
* **Severity levels**: Use [CRITICAL], [HIGH], [MEDIUM], [LOW] tags
* Block PRs on [CRITICAL] and [HIGH] issues
* For security issues, consider discussing privately before commenting publicly

## Final Check
Before approving, ask yourself:
1. Could this brick the chain? (panic, consensus break)
2. Could this lose or steal funds? (arithmetic, logic errors)
3. Could this DOS the network? (unbounded operations, weight issues)
4. Could this introduce a backdoor? (especially for external contributors)
5. Is this change necessary and minimal?

**Remember: $4B market cap. Err on the side of caution. When in doubt, escalate.**

@sam0x17 sam0x17 merged commit 0b7ff00 into devnet-ready Oct 16, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-cargo-audit This PR fails cargo audit but needs to be merged anyway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants