From d917faf7b5f89e30ba2cafbfcf06629313515608 Mon Sep 17 00:00:00 2001
From: bootc-dev Bot <bot@bootc.dev>
Date: Thu, 23 Jul 2026 21:09:17 +0000
Subject: [PATCH] Sync common files from infra repository

Synchronized from bootc-dev/infra@0c34c5b394926053b2fbe1106320f37bba500afa.

Signed-off-by: bootc-dev Bot <bot@bootc.dev>
---
 .bootc-dev-infra-commit.txt |   2 +-
 AGENTS.md                   |   5 +-
 REVIEW.md                   |  46 +++++++------
 REVIEW_GOLANG.md            | 130 ++++++++++++++++++++++++++++++++++++
 REVIEW_RUST.md              |  60 +++++++++++++++++
 5 files changed, 219 insertions(+), 24 deletions(-)
 create mode 100644 REVIEW_GOLANG.md
 create mode 100644 REVIEW_RUST.md

diff --git a/.bootc-dev-infra-commit.txt b/.bootc-dev-infra-commit.txt
index 00cf47f..4a51c41 100644
--- a/.bootc-dev-infra-commit.txt
+++ b/.bootc-dev-infra-commit.txt
@@ -1 +1 @@
-55772cd1ed6efa2f315f6a1cb03b80c575037932
+0c34c5b394926053b2fbe1106320f37bba500afa
diff --git a/AGENTS.md b/AGENTS.md
index 1366e52..6126130 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -57,7 +57,10 @@ the commit message text.
 ## Code guidelines
 
 The [REVIEW.md](REVIEW.md) file describes expectations around
-testing, code quality, commit messages, commit organization, etc. If you're
+testing, code quality, commit messages, commit organization, etc.
+Language-specific guidelines are in
+[REVIEW_RUST.md](REVIEW_RUST.md) and
+[REVIEW_GOLANG.md](REVIEW_GOLANG.md). If you're
 creating a change, it is strongly encouraged after each 
 commit and especially when the agent thinks a task is complete
 to spawn a subagent to perform a review using guidelines (alongside
diff --git a/REVIEW.md b/REVIEW.md
index 3c1d690..3068dd5 100644
--- a/REVIEW.md
+++ b/REVIEW.md
@@ -26,9 +26,10 @@ 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.
+reading: have the parser accept the raw data (e.g. a string), then have a
+separate function that reads from disk and calls the parser. This makes unit
+testing straightforward without filesystem dependencies. See the
+language-specific review guides for concrete examples.
 
 ### Test Assertions
 
@@ -48,9 +49,7 @@ or `sed`.
 
 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.
+level ways to structure things (Justfile e.g.).
 
 ### Constants and Magic Values
 
@@ -64,10 +63,10 @@ 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.
+Avoid swallowing errors (e.g. `foo 2>/dev/null || true` in shell script).
+Most errors should be propagated by default. If not, it's usually appropriate
+to at least log error messages at a debug level. See the language-specific
+review guides for concrete anti-patterns.
 
 Handle edge cases explicitly: missing data, malformed input, offline systems.
 Error messages should provide clear context for diagnosis.
@@ -192,21 +191,17 @@ functionality, ensure equivalent coverage exists.
 
 When multiple contributors co-author a PR, bring in an independent reviewer.
 
-## Rust-Specific Guidance
+## Dependencies
 
-Prefer rustix over `libc`. All `unsafe` code must be very carefully
-justified.
+New dependencies should be justified. Consider alternatives: "I'm curious if
+you did any comparative analysis at all with alternatives?"
 
-### Dependencies
+Prefer well-maintained libraries with active communities. Glance at existing
+reverse dependencies to gauge adoption (e.g. on crates.io for Rust, or
+pkg.go.dev for Go). Consider project-level dependency policies (e.g.
+`cargo deny` for Rust).
 
-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
+## API Design
 
 When adding new commands or options, think about machine-readable output early.
 JSON is generally preferred for that.
@@ -214,3 +209,10 @@ 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.
+
+## Language-Specific Guidance
+
+The following guides cover language-specific review expectations:
+
+- [REVIEW_RUST.md](REVIEW_RUST.md) — Rust projects
+- [REVIEW_GOLANG.md](REVIEW_GOLANG.md) — Go projects
diff --git a/REVIEW_GOLANG.md b/REVIEW_GOLANG.md
new file mode 100644
index 0000000..1199339
--- /dev/null
+++ b/REVIEW_GOLANG.md
@@ -0,0 +1,130 @@
+# Go-Specific Review Guidelines
+
+These guidelines supplement the general [REVIEW.md](REVIEW.md) with
+Go-specific expectations.
+
+## Separating Parsing from I/O
+
+Have the parser accept a string or `io.Reader`, then have a separate function
+that opens the file and calls the parser:
+
+```go
+// ✅ Good: parser is a pure function, easy to unit test
+func parseConfig(data string) (*Config, error) { ... }
+
+func loadConfig(path string) (*Config, error) {
+    data, err := os.ReadFile(path)
+    if err != nil {
+        return nil, err
+    }
+    return parseConfig(string(data))
+}
+```
+
+## Don't Ignore (Swallow) Errors
+
+Avoid discarding errors with the blank identifier. Most errors should be
+returned to the caller. If not, at least log the error:
+
+```go
+// ❌ Avoid: error is silently swallowed
+_ = doSomething()
+
+// ✅ Good: propagate
+if err := doSomething(); err != nil {
+    return err
+}
+
+// ✅ OK if the error is truly ignorable: log it
+if err := doSomething(); err != nil {
+    log.Debug("ignoring error", "err", err)
+}
+```
+
+## Gomega and Test Assertions
+
+We use [gomega](https://github.com/onsi/gomega) for test assertions. Follow
+these conventions:
+
+### Use `g.Eventually` for Polling
+
+Gomega's `Eventually` handles polling, timeouts, and failure reporting.
+
+### Return `(T, error)` from `Eventually` Callbacks
+
+Return the specific field you care about and let gomega matchers describe the
+expectation declaratively. This produces better failure messages because
+gomega can show what the value actually was vs. what was expected.
+
+```go
+// ✅ Good: return the field, match with gomega
+g.Eventually(func() ([]metav1.Condition, error) {
+    var p bootcv1alpha1.BootcNodePool
+    err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, &p)
+    return p.Status.Conditions, err
+}).Should(ContainElement(And(
+    HaveField("Type", bootcv1alpha1.PoolDegraded),
+    HaveField("Status", metav1.ConditionTrue),
+    HaveField("Reason", bootcv1alpha1.PoolNodeDegraded),
+)))
+
+// ❌ Avoid: assertions inside the callback with Succeed()
+g.Eventually(func(g Gomega) {
+    var p bootcv1alpha1.BootcNodePool
+    g.Expect(k8sClient.Get(ctx, ...)).To(Succeed())
+    cond := apimeta.FindStatusCondition(p.Status.Conditions, ...)
+    g.Expect(cond).NotTo(BeNil())
+    g.Expect(cond.Status).To(Equal(...))
+}).Should(Succeed())
+```
+
+### Return the Narrowest Type
+
+Extract exactly the field you want to assert on — labels, conditions,
+ownerReference — rather than returning the whole object or a `bool`:
+
+```go
+// Labels
+g.Eventually(func() (map[string]string, error) {
+    var n corev1.Node
+    err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, &n)
+    return n.Labels, err
+}).Should(HaveKey(bootcv1alpha1.LabelManaged))
+
+// OwnerReference
+g.Eventually(func() (*metav1.OwnerReference, error) {
+    var bn bootcv1alpha1.BootcNode
+    err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, &bn)
+    return metav1.GetControllerOf(&bn), err
+}).Should(And(Not(BeNil()), HaveField("Name", pool.Name)))
+```
+
+### Use Composed Matchers for Struct Assertions
+
+Prefer `HaveField` and `ContainElement(And(...))` to match on struct fields
+declaratively rather than manually extracting fields and asserting one by one:
+
+```go
+// ✅ Good: declarative, one expression
+g.Expect(conditions).To(ContainElement(And(
+    HaveField("Type", bootcv1alpha1.PoolDegraded),
+    HaveField("Status", metav1.ConditionTrue),
+    HaveField("Reason", bootcv1alpha1.PoolInvalidSpec),
+)))
+
+// ❌ Avoid: manual lookup + sequential field assertions
+cond := apimeta.FindStatusCondition(conditions, bootcv1alpha1.PoolDegraded)
+g.Expect(cond).NotTo(BeNil())
+g.Expect(cond.Status).To(Equal(metav1.ConditionTrue))
+g.Expect(cond.Reason).To(Equal(bootcv1alpha1.PoolInvalidSpec))
+```
+
+### Assert Specific Errors When Expected
+
+When a test expects a particular error, match on the concrete error type or
+value:
+
+```go
+// ✅ Good: we know the API server should reject this
+g.Expect(err).To(MatchError(apierrors.IsInvalid, "IsInvalid"))
+```
diff --git a/REVIEW_RUST.md b/REVIEW_RUST.md
new file mode 100644
index 0000000..ac7983b
--- /dev/null
+++ b/REVIEW_RUST.md
@@ -0,0 +1,60 @@
+# Rust-Specific Review Guidelines
+
+These guidelines supplement the general [REVIEW.md](REVIEW.md) with
+Rust-specific expectations.
+
+## Separating Parsing from I/O
+
+Have the parser accept a `&str`, then have a separate function that reads from
+disk and calls the parser:
+
+```rust
+// ✅ Good: parser is a pure function, easy to unit test
+fn parse_config(data: &str) -> Result<Config> { ... }
+
+fn load_config(path: &Path) -> Result<Config> {
+    let data = std::fs::read_to_string(path)?;
+    parse_config(&data)
+}
+```
+
+## Don't Ignore (Swallow) Errors
+
+Avoid the `if let Ok(v) = ... { }` pattern which silently discards the error
+branch. Most errors should be propagated with `?`. If not, at least log the
+error:
+
+```rust
+// ❌ Avoid: error is silently swallowed
+if let Ok(v) = do_something() {
+    use_value(v);
+}
+
+// ✅ Good: propagate
+let v = do_something()?;
+
+// ✅ OK if the error is truly ignorable: log it
+match do_something() {
+    Ok(v) => use_value(v),
+    Err(e) => tracing::debug!("ignoring error: {e}"),
+}
+```
+
+## Shell Scripts
+
+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.
+This is preferred over long shell scripts.
+
+## Code Formatting
+
+After making changes to any .rs files, `cargo fmt` should be run.
+There are CI jobs that lint and expect `cargo fmt` to be a no-op.
+Failing to properly format rust code will result in wasted CI
+resources.
+
+## General
+
+Prefer rustix over `libc`. All `unsafe` code must be very carefully
+justified.
+