From f35bdea021f37d1ef293f6f2467216806afffec8 Mon Sep 17 00:00:00 2001 From: bootc-dev Bot Date: Mon, 11 May 2026 04:53:53 +0000 Subject: [PATCH] Sync common files from infra repository Synchronized from bootc-dev/infra@ff66c1c724faeb83b5cd2af36d9a5d2129ecf95a. Signed-off-by: bootc-dev Bot --- .bootc-dev-infra-commit.txt | 1 + .claude/CLAUDE.md | 1 + .cursorrules | 1 + .devcontainer/debian/devcontainer.json | 2 +- .devcontainer/devcontainer.json | 2 +- .devcontainer/ubuntu/devcontainer.json | 2 +- .gemini/config.yaml | 19 +++ AGENTS.md | 41 +++++ REVIEW.md | 216 +++++++++++++++++++++++++ 9 files changed, 282 insertions(+), 3 deletions(-) create mode 100644 .bootc-dev-infra-commit.txt create mode 120000 .claude/CLAUDE.md create mode 120000 .cursorrules create mode 100644 .gemini/config.yaml create mode 100644 AGENTS.md create mode 100644 REVIEW.md diff --git a/.bootc-dev-infra-commit.txt b/.bootc-dev-infra-commit.txt new file mode 100644 index 000000000..47dceb5c4 --- /dev/null +++ b/.bootc-dev-infra-commit.txt @@ -0,0 +1 @@ +ff66c1c724faeb83b5cd2af36d9a5d2129ecf95a diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md new file mode 120000 index 000000000..be77ac83a --- /dev/null +++ b/.claude/CLAUDE.md @@ -0,0 +1 @@ +../AGENTS.md \ No newline at end of file diff --git a/.cursorrules b/.cursorrules new file mode 120000 index 000000000..47dc3e3d8 --- /dev/null +++ b/.cursorrules @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/.devcontainer/debian/devcontainer.json b/.devcontainer/debian/devcontainer.json index 12d52a3c2..f1b43a85c 100644 --- a/.devcontainer/debian/devcontainer.json +++ b/.devcontainer/debian/devcontainer.json @@ -1,5 +1,5 @@ { - "name": "bootcrew-devenv-debian", + "name": "bootc-devenv-debian", "image": "ghcr.io/bootc-dev/devenv-debian", "customizations": { "vscode": { diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 12d52a3c2..f1b43a85c 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -1,5 +1,5 @@ { - "name": "bootcrew-devenv-debian", + "name": "bootc-devenv-debian", "image": "ghcr.io/bootc-dev/devenv-debian", "customizations": { "vscode": { diff --git a/.devcontainer/ubuntu/devcontainer.json b/.devcontainer/ubuntu/devcontainer.json index a5d6e703c..8a6167ace 100644 --- a/.devcontainer/ubuntu/devcontainer.json +++ b/.devcontainer/ubuntu/devcontainer.json @@ -1,5 +1,5 @@ { - "name": "bootcrew-devenv-ubuntu", + "name": "bootc-devenv-ubuntu", "image": "ghcr.io/bootc-dev/devenv-ubuntu", "customizations": { "vscode": { diff --git a/.gemini/config.yaml b/.gemini/config.yaml new file mode 100644 index 000000000..080ba110c --- /dev/null +++ b/.gemini/config.yaml @@ -0,0 +1,19 @@ +# NOTE: This file is canonically maintained in +# +# DO NOT EDIT +# +# This config mainly overrides `summary: false` by default +# as it's really noisy. +have_fun: true +code_review: + disable: false + # Even medium level can be quite noisy, I don't think + # we need LOW. Anyone who wants that type of stuff should + # be able to get it locally or before review. + comment_severity_threshold: MEDIUM + max_review_comments: -1 + pull_request_opened: + help: false + summary: false # turned off by default + code_review: true +ignore_patterns: [] diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..fffc7b62c --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,41 @@ + + +# Instructions for AI agents + +## CRITICAL instructions for generating commits + +### Signed-off-by + +Human review is required for all code that is generated +or assisted by a large language model. If you +are a LLM, you MUST NOT include a `Signed-off-by` +on any automatically generated git commits. Only explicit +human action or request should include a Signed-off-by. +If for example you automatically create a pull request +and the DCO check fails, tell the human to review +the code and give them instructions on how to add +a signoff. + +### Attribution + +When generating substantial amounts of code, you SHOULD +include an `Assisted-by: TOOLNAME (MODELNAME)`. For example, +`Assisted-by: Goose (Sonnet 4.5)`. + +## Code guidelines + +The [REVIEW.md](REVIEW.md) file describes expectations around +testing, code quality, commit messages, commit organization, etc. If you're +creating a change, it is strongly encouraged after each +commit and especially when you think a task is complete +to spawn a subagent to perform a review using guidelines (alongside +looking for any other issues). + +If you are performing a review of other's code, the same +principles apply. + +## Follow other guidelines + +Look at the project README.md and look for guidelines +related to contribution, such as a CONTRIBUTING.md +and follow those. diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 000000000..3c1d690f5 --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,216 @@ +# Code Review Guidelines + +These guidelines are derived from analysis of code reviews across the bootc-dev +organization (October–December 2024). They represent the collective expectations +and standards that have emerged from real review feedback. + +## Testing + +Tests are expected for all non-trivial changes - unit and integration by default. + +If there's something that's difficult to write a test for at the current time, +please do at least state if it was tested manually. + +### Choosing the Right Test Type + +Unit tests are appropriate for parsing logic, data transformations, and +self-contained functions. Use integration tests for anything that involves +running containers or VMs. + +Default to table-driven tests instead of having a separate unit test per +case. Especially LLMs like to generate the latter, but it can become +too verbose. Context windows matter to both humans and LLMs reading the +code later (this applies outside of unit tests too of course, but it's +easy to generate a *lot* of code for unit tests unnecessarily). + +### Separating Parsing from I/O + +A recurring theme is structuring code for testability. Split parsers from data +reading: have the parser accept a `&str`, then have a separate function that +reads from disk and calls the parser. This makes unit testing straightforward +without filesystem dependencies. + +### Test Assertions + +Make assertions strict and specific. Don't just verify that code "didn't crash"— +check that outputs match expected values. When adding new commands or output +formats, tests should verify the actual content, not just that something was +produced. + +## Code Quality + +### Parsing Structured Data + +Never parse structured data formats (JSON, YAML, XML) with text tools like `grep` +or `sed`. + +### Shell Scripts + +Try to avoid having shell script longer than 50 lines. This commonly occurs +in build system and tests. For the build system, usually there's higher +level ways to structure things (Justfile e.g.) and several of our projects +use the `cargo xtask` pattern to put arbitrary "glue" code in Rust using +the `xshell` crate to keep it easy to run external commands. + +### Constants and Magic Values + +Extract magic numbers into named constants. Any literal number that isn't +immediately obvious—buffer sizes, queue lengths, retry counts, timeouts—should +be a constant with a descriptive name. The same applies to magic strings: +deduplicate repeated paths, configuration keys, and other string literals. + +When values aren't self-explanatory, add a comment explaining why that specific +value was chosen. + +### Don't ignore (swallow) errors + +Avoid the `if let Ok(v) = ... { }` in Rust, or `foo 2>/dev/null || true` +pattern in shell script by default. Most errors should be propagated by +default. If not, it's usually appropriate to at least log error messages +at a `tracing::debug!` or equivalent level. + +Handle edge cases explicitly: missing data, malformed input, offline systems. +Error messages should provide clear context for diagnosis. + +### Code Organization + +Separate concerns: I/O operations, parsing logic, and business logic belong in +different functions. Structure code so core logic can be unit tested without +external dependencies. + +It can be OK to duplicate a bit of code in a slightly different form twice, +but having it happen in 3 places asks for deduplication. + +## Commits and Pull Requests + +### Commit Organization + +Break changes into logical, atomic commits. Reviewers appreciate being able to +follow your reasoning: "Especially grateful for breaking it up into individual +commits so I can more easily follow your train of thought." + +Preparatory refactoring should be separate from behavioral changes. Each commit +should tell a clear story and be reviewable independently. Where applicable, +create "prep" commits that could be merged separately from the behavioral change. + +### Commit Messages + +Write clear and descriptive commit messages using a `component: Summary` +subject, such as `kernel: Add find API w/correct hyphen-dash equality, add docs`. +Use imperative mood: "Add integration with..." not "Adds integration with...". + +The body of the commit should start with at least one sentence (or paragraph) +describing **why** the change is being made, even for something apparently +trivial. For example a "refactor" commit might have a "why" rationale of just +"Prep for handling X later." A big commit introducing a feature may seem +self-explanatory, but there is often ambient context like "A large-scale Debian +user wanted this" that provides helpful grounding in the motivation. + +If there's a linked tracking issue, often that will contain a more extensive +rationale that doesn't need to be duplicated entirely in the commit message, +but do ensure the commit message has something useful on its own for a rationale. + +Keep it natural and concise. A few sentences of prose explaining the design +intent or the high-level data flow is often good enough. If there's a +non-obvious consequence of the change, call it out briefly (e.g. "Note the +manifest becomes part of the GC root") rather than explaining the full +mechanism. Think about what a reviewer needs to know that may not be obvious +from a skim of the code. + +Do not restate obvious parts of what is already visible in the commit diff: + +- "Changed function X to call Y" +- Generic `Changes:` sections with bulleted lists of implementation details +- "Files changed" sections — completely redundant with git + +Implementation details belong in the code documentation. The goal of the +commit message is like a "cover letter" for the change, with a primary +rationale of why the change is being made, alongside a concise summary of +its implementation. + +Another thing that can go in the commit message is brief descriptions +of alternative approaches that were considered and discarded. + +Closes: tags should generally come at the end of the commit message. + +### PR Descriptions + +Generally, just restate the commit message. + +Where it makes sense, it is OK to include additional details though. + +### Further changes on top of existing commits + +If you have followup fixes (whether that's part of a local loop or +as part of addressing PR review), it is generally encouraged to *squash* +the fixes into the prior commit. Do not create generically-named "Update " commits +or "Address review feedback" or "Fix cargo fmt" commits. + +This applies equally when an AI tool (e.g. Gemini, Copilot) suggests a +change via a review comment — applying the suggestion creates a new commit +with an auto-generated subject. That commit should be squashed before the +PR is merged. + +In other words either a commit "stands alone" with its own rationale or it doesn't. + +### Keeping PRs Current + +Keep PRs rebased on main. When CI failures are fixed in other PRs, rebase to +pick up the fixes. Reference the fixing PR when noting that a rebase is needed. + +### Before Merge + +Self-review your diff before requesting review. Catch obvious issues yourself +rather than burning reviewer cycles. + +Do not add `Signed-off-by` lines automatically—these require explicit human +action after review. If code was AI-assisted, include an `Assisted-by:` trailer +indicating the tool and model used. + +## Architecture and Design + +### Workarounds vs Proper Fixes + +When implementing a workaround, document where the proper fix belongs and link +to relevant upstream issues. Invest time investigating proper fixes before +settling on workarounds. + +### Cross-Project Considerations + +Prefer pushing fixes upstream when the root cause is in a dependency. Reduce +scope where possible; don't reimplement functionality that belongs elsewhere. + +When multiple systems interact (like Renovate and custom sync tooling), be +explicit about which system owns what and how they coordinate. + +### Avoiding Regressions + +Verify that new code paths handle all cases the old code handled. When rewriting +functionality, ensure equivalent coverage exists. + +### Review Requirements + +When multiple contributors co-author a PR, bring in an independent reviewer. + +## Rust-Specific Guidance + +Prefer rustix over `libc`. All `unsafe` code must be very carefully +justified. + +### Dependencies + +New dependencies should be justified. Glance at existing reverse dependencies +on crates.io to see if a crate is widely used. Consider alternatives: "I'm +curious if you did any comparative analysis at all with alternatives?" + +Prefer well-maintained crates with active communities. Consider `cargo deny` +policies when adding dependencies. + +### API Design + +When adding new commands or options, think about machine-readable output early. +JSON is generally preferred for that. + +Keep helper functions in appropriate modules. Move command output formatting +close to the CLI layer, keeping core logic functions focused on their primary +purpose.