Skip to content

Conversation

@angiejones
Copy link
Collaborator

Summary

Type of Change

  • Feature
  • Bug fix
  • Refactor / Code quality
  • Performance improvement
  • Documentation
  • Tests
  • Security fix
  • Build / Release
  • Other (specify below)

AI Assistance

  • This PR was created or reviewed with AI assistance

Copilot AI review requested due to automatic review settings November 9, 2025 21:18
Copy link
Contributor

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 comprehensive GitHub Copilot code review instructions to guide AI-assisted code reviews for the project.

  • Establishes clear review philosophy emphasizing high-confidence feedback and actionable comments
  • Defines priority areas including security, correctness, architecture patterns, and testing gaps
  • Provides project-specific context for Rust workspace structure and tooling requirements

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@michaelneale michaelneale left a comment

Choose a reason for hiding this comment

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

looks good @angiejones where did you get those guidelines from?

I think over time if we see it flag things incorrectly which are due to specific quirks of how goose works vs other style of apps, we can easily add the tips to here.

I likeit.

Copy link
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

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

thanks for doing this, we do need something here. I wonder though if we should just all go through the reviews we got from copilot and collect what was useful and what was not.

@DOsinga
Copy link
Collaborator

DOsinga commented Nov 10, 2025

fwiw, the three comments I got on this:

#5643

were all spot on

@tlongwell-block
Copy link
Collaborator

tlongwell-block commented Nov 10, 2025

This is a great idea. Something to keep in mind, copilot does read AGENTS.md in the root and in subdirectories. Though hard to tell if that is just the "coding" agent, or copilot in general, including the review bot.

(Maybe goose should check subdirs too when it is working. That would be a nice feature that would allow per-crate instructions, @DOsinga?)

So, we could probably reduce duplication a bit between the instructions file here and agents.

@DOsinga
Copy link
Collaborator

DOsinga commented Nov 10, 2025

I actually have some notes on what goes often wrong in our code:

Do not make things optional that don't need to be. The compiler is annoying but is your friend
Remove useless LLM comments - at best they take up space, at worst they will be outdated soon and will confuse
Beware for Goose suggesting to check 3 different things. It is probably only one
Booleans should default to false and not be optional
Beware of the context manager that doesn't need context
Clean up your logging
Avoid overly defensive code

@tlongwell-block
Copy link
Collaborator

tlongwell-block commented Nov 10, 2025

I actually have some notes on what goes often wrong in our code:

Do not make things optional that don't need to be. The compiler is annoying but is your friend Remove useless LLM comments - at best they take up space, at worst they will be outdated soon and will confuse Beware for Goose suggesting to check 3 different things. It is probably only one Booleans should default to false and not be optional Beware of the context manager that doesn't need context Clean up your logging Avoid overly defensive code

I would probably put this in AGENTS.md so all AI working on the repo will follow these guidelines?

Looks like copilot review does respect agents, too https://docs.github.com/en/copilot/concepts/agents/code-review#providing-instructions-for-copilot-code-reviews

@DOsinga
Copy link
Collaborator

DOsinga commented Nov 10, 2025

it's a good point that we include AGENTS.md anyway, but I would still think repeating the important bits as explicit code review instructions (pay attention to optional things that shouldn't) wouldn't hurt

@angiejones
Copy link
Collaborator Author

angiejones commented Nov 10, 2025

it's a good point that we include AGENTS.md anyway, but I would still think repeating the important bits as explicit code review instructions (pay attention to optional things that shouldn't) wouldn't hurt

added coding guidelines to AGENTS.md and review guidelines to the copilot custom instructions
2ccc3a7

@angiejones angiejones merged commit a971a64 into main Nov 10, 2025
16 checks passed
tiensi added a commit to tiensi/goose that referenced this pull request Nov 11, 2025
* main: (83 commits)
  silence copilot on minor text issues (block#5665)
  fix: disallow runaway subagent chains (block#5659)
  chore: remove usage of non-existent env var for log dir (block#5658)
  clarify agent instructions (block#5655)
  feat: add check-everything for unified style checks (block#5650)
  Show errors on failure (block#5643)
  custom instructions for copilot reviews (block#5646)
  fix: prevent repeated 404 errors when accessing deleted sessions (block#5644)
  Flake.nix corrected main (block#5600)
  fix: goose recipe list can return duplicated entries (block#5645)
  fix: bedrock creds refresh (block#5599)
  Fix Claude Code provider to default to Auto mode (block#5638) (block#5642)
  Scheduler cleanup (block#5571)
  Better search paths and handling of CLI providers (block#5554)
  docs: description required for "Add Extension" in cli - phase 2 (block#5635)
  Remove some logging (block#5631)
  Use session IDs as task IDs for subagents instead of UUIDs (block#5398)
  Fix the naming (block#5628)
  fix: default tetrate model is broken, replace with haiku-4.5 (block#5535) (block#5587)
  Fetch less and use the right SHA (block#5621)
  ...
michaelneale added a commit that referenced this pull request Nov 11, 2025
* main:
  silence copilot on minor text issues (#5665)
  fix: disallow runaway subagent chains (#5659)
  chore: remove usage of non-existent env var for log dir (#5658)
  clarify agent instructions (#5655)
  feat: add check-everything for unified style checks (#5650)
  Show errors on failure (#5643)
  custom instructions for copilot reviews (#5646)
  fix: prevent repeated 404 errors when accessing deleted sessions (#5644)
  Flake.nix corrected main (#5600)
  fix: goose recipe list can return duplicated entries (#5645)
  fix: bedrock creds refresh (#5599)
Surendhar-N-D pushed a commit to Surendhar-N-D/goose that referenced this pull request Nov 17, 2025
arul-cc pushed a commit to arul-cc/goose that referenced this pull request Nov 17, 2025
BlairAllan pushed a commit to BlairAllan/goose that referenced this pull request Nov 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants