code-review

---
name: code-review
description: Review AgentBaker pull requests and diffs for production regressions, backward compatibility, security, architecture, cross-OS behavior, provisioning failures, and risky package updates. Use this skill whenever reviewing AgentBaker code, pull requests, diffs, dependency bumps, or changes to Linux or Windows VHD build and node provisioning paths.
---

# AgentBaker Code Review

When reviewing pull requests, perform breaking change analysis to prevent regressions. VHDs remain in production for 6 months, so backward compatibility is critical.

Focus on architecture, security vulnerabilities, and logic errors. Understand the code's intent, dependencies, and concrete failure modes before you report a finding.

## Breaking Change Detection

Analyze PRs for these compatibility scenarios:

**1. Linux Provisioning Script Changes**

- **Context**: Scripts in `parts/linux/cloud-init/artifacts/` run during critical VM bootstrap and are used in both:
  - VHD build (uploaded via packer configs in `vhdbuilder/packer/*.json`)
  - VM provisioning (CSE - embedded in Go service via `pkg/agent/const.go`)
  - Versions synchronized via `pkg/agent/datamodel/linux_sig_version.json`
- **What to check**: Changes that could break VM provisioning in production
- **Breaking signals**:
  - **Script logic errors**: Syntax errors, wrong commands, incorrect flags, broken pipes
  - **Dependency issues**:
    - Calling functions before they're sourced
    - Using variables declared in other functions
    - Removing `source` statements that break dependency chains
  - **Cross-distro compatibility**:
    - Commands that don't work on both Ubuntu and Azure Linux/Mariner (check distro-specific variants: `ubuntu/`, `mariner/`)
    - Package manager assumptions (apt vs dnf/tdnf)
    - Missing OS-specific conditional logic
    - Systemd differences between distributions
  - **External dependency violations**:
    - Downloading from internet URLs not in `parts/common/components.json` or allowed sources (packages.aks.azure.com)
    - All external dependencies MUST be referenced in `parts/common/components.json` for Renovate updates
    - Only allowed runtime downloads: packages.aks.azure.com or other explicitly allowed sources in CSE
  - **Function signature changes**: Parameters, return values, exit codes that break callers
  - **ANC hotfix entry removal**: If a PR removes or modifies the `hotfix-scripts: auto-generated` block in `parts/linux/cloud-init/nodecustomdata.yml`, or resets `parts/linux/cloud-init/artifacts/aks-node-controller-hotfix.json` to `{}`, **always confirm with the PR owner** that all affected VHDs have been republished with the fix baked in or are out of the 6-month support window. Premature removal means nodes provisioned via scale-up on the old buggy VHD will no longer receive the hotfix. These files are auto-generated by `hotfix/hotfix_generate.py` (via the `hotfix-generate` GH Action) — see that script for how `version`/`scripts_version` are computed.

**2. Windows Bidirectional Compatibility**

- **Context**: Windows VHD and CSE scripts release on different cadences with no guaranteed order
- **What to check**: Changes to `staging/cse/windows/` (CSE scripts) or `vhdbuilder/packer/windows/` (VHD scripts)
- **Breaking signals**:
  - New CSE scripts assuming capabilities that old VHDs don't have
  - New VHD scripts expecting features that old CSE versions don't provide
  - Changes to shared state (registry keys, files, environment variables) that break coordination
  - Removing PowerShell functions or cmdlets that the other component might call

**3. aks-node-controller Migration (Dual-Mode Support)**

- **Context**: Transitioning from uploading scripts during both VHD build and CSE to only uploading aks-node-controller during VHD build
- **What to check**: Any changes must work in BOTH deployment modes
- **Breaking signals**:
  - Assumptions that scripts are always uploaded during CSE (new mode won't do this)
  - Assumptions that aks-node-controller is always present (old VHDs won't have it)
  - Missing feature detection to determine which mode is running
  - Hardcoded paths that differ between deployment modes

**4. PIS / VHD Caching — basePrep vs nodePrep split (Windows + Linux)**

- **Context**: PIS bakes a VHD from a temporary VM, then boots many real nodes from it. Same model in Windows `parts/windows/kuberneteswindowssetup.ps1.template` (`BasePrep`/`NodePrep`) and Linux `parts/linux/cloud-init/artifacts/cse_main.sh` (`basePrep`/`nodePrep`):
  - `basePrep` — gated only by the `base_prep.complete` marker (`C:\AzureData\` Windows, `/opt/azure/containers/` Linux). **Skipped on PIS real nodes** because the marker is baked into the VHD.
  - `nodePrep` — runs whenever `PreProvisionOnly` is false (every real node); skipped only on the bake VM.
  - The bake run sets `PreProvisionOnly=true` (`{{GetPreProvisionOnly}}`) and writes the marker after `basePrep` succeeds (Windows `finally`; Linux `cse_start.sh`).
  - On PIS, basePrep and nodePrep are separate VM runs: variables re-initialize from the real node's live CustomData, so `basePrep` in-memory state is gone. (Non-PIS: both run in one execution, no reboot between phases, so state carries — but don't rely on it.)
- **Breaking signals**:
  - Perishable data written in `basePrep`: TLS bootstrap tokens, any kubeconfig embedding a token (e.g. Windows `Write-BootstrapKubeConfig` → `c:\k\bootstrap-config`), secrets, expiring SAS URLs/creds, per-node identity/name/certs. Baked into the VHD and never refreshed (real nodes skip `basePrep`) → stale. Write it in `nodePrep`.
  - `nodePrep` reading a variable that only `basePrep` set (not re-derived from CustomData) — empty on PIS nodes.
  - A service/scheduled task enabled in `basePrep` that auto-starts on the real node before `nodePrep` refreshes its config — it runs against the stale baked config (e.g. a kubelet unit starting from the bake-time `bootstrap-config` before `nodePrep` rewrites the token).
  - Heavy node-agnostic work (package/binary downloads, image pulls) moved into `nodePrep` — defeats caching, slows provisioning. Keep it in `basePrep`.
- **Rule**: `basePrep` = static, non-secret, valid for every node booting weeks later. `nodePrep` = node-specific, secret, expiring, or CustomData-derived.
- **Don't flag**: plain variable reads (live on the real node from CustomData); cached binaries/packages/images; the `base_prep.complete` marker; cluster-wide non-secrets (CA cert, apiserver FQDN, service CIDR); pre-existing `basePrep` writes unless the PR newly depends on them.

**5. Package/Dependency Update PRs (Renovate)**

- **Context**: Renovate bot automatically creates PRs to update component versions in `parts/common/components.json`. These components are cached on VHDs during build and directly affect node stability, GPU workloads, networking, and security. Updated packages are downloaded from `packages.aks.azure.com` or upstream registries during VHD build.
- **What to check**: Every version bump—even patch versions—can introduce regressions that affect production nodes.
- **Analysis steps for every package update PR**:
  1. **Identify the component and version change**: Parse the diff in `parts/common/components.json` to extract exact old → new versions for each OS/release entry.
  2. **Determine the update type**: Classify as major, minor, or patch using semver. Major and minor updates carry higher risk than patch updates.
  3. **Research upstream changelog**: Look up the project's release notes, changelog, or GitHub releases to understand what changed between the old and new versions. Summarize:
     - New features introduced
     - Bug fixes included
     - Breaking changes or deprecations
     - Security fixes (CVEs patched)
  4. **Assess OS coverage**: Check if the update covers all OS variants where the component is used (Ubuntu 22.04, 24.04, Azure Linux 3.0, etc.). Flag if some OS entries are updated but others are not — partial updates can cause inconsistency across node pools.
  5. **Evaluate VHD size impact**: For components downloaded as binaries or packages, consider whether the new version significantly increases VHD size. Large size increases can affect VHD build time and storage costs.
  6. **Check for configuration or API changes**: If the component exposes configuration files, CLI flags, systemd units, or APIs consumed by CSE scripts, verify that the update doesn't change defaults or remove options that provisioning scripts depend on.
  7. **Verify download URL validity**: Confirm that the `downloadLocation` and `downloadURIs` structure in components.json remains valid for the new version. New versions sometimes change the artifact naming convention or repository layout.
  8. **Verify AKS-RP coordination**: AKS-RP is the Azure Kubernetes Service Resource Provider; it orchestrates cluster and node-pool operations and tells nodes which component versions to use. Check production code (for example, with internal Azure DevOps code search) to determine whether AKS-RP already requests, or is about to request, the new version and whether the bump removes a cached version it still requests. Compare those requests with every version that will remain in `components.json`, including `latestVersion` and `previousLatestVersion`, and ignore testdata and fixtures. First distinguish components for which AKS-RP selects a version from components that simply use the version baked into the VHD; the latter do not require version coordination. If AKS-RP access is unavailable, state that this check was not completed and request confirmation from the component owner rather than assuming coordination. Passing CI is not proof of coordination: a missing cached version can still be downloaded at runtime, allowing provisioning to succeed with increased latency.

- **Risk assessment for package updates**:
  - 🔴 **High Risk**: Major version bumps, components critical to node boot (kubelet, containerd, runc), GPU drivers (nvidia-driver, dcgm-exporter), or networking (azure-cni, cilium). Also high risk if upstream changelog mentions breaking changes or behavioral changes.
  - 🟡 **Medium Risk**: Minor version bumps of non-critical components, updates that only affect specific OS variants, or updates where upstream changelog shows feature additions that could subtly change behavior.
  - 🟢 **Low Risk**: Patch version bumps with only bug fixes or security patches, no breaking changes in upstream changelog, and full OS coverage.

- **Review output for package update PRs must include a detailed version diff analysis**:

  **Header:**

  ```
  ## Package Update Analysis: <component-name>
  **Version change**: X.Y.Z → A.B.C (<major|minor|patch> update)
  **OS variants affected**: Ubuntu 22.04, Ubuntu 24.04, Azure Linux 3.0 (list all)
  **OS variants NOT updated**: <list any missing, or "None — full coverage">
  ```

  **Detailed changelog between versions:**
  Use web search, GitHub releases, or upstream project documentation to find the exact differences between the old and new version. Present each change as a line item with its own risk tag:

  ```
  ### Changes between X.Y.Z and A.B.C

  | Change | Description | Risk |
  |--------|-------------|------|
  | Feature | <brief description of new feature> | 🟢 Low / 🟡 Medium / 🔴 High |
  | Bug fix | <brief description of bug fixed> | 🟢 Low / 🟡 Medium / 🔴 High |
  | Breaking | <description of breaking change> | 🔴 High |
  | Security | CVE-YYYY-XXXXX: <description> | 🟢 Low / 🟡 Medium / 🔴 High |
  | Deprecation | <what was deprecated and migration path> | 🟡 Medium / 🔴 High |
  | Config change | <default value changed or option removed> | 🟡 Medium / 🔴 High |
  | Performance | <perf improvement or regression> | 🟢 Low / 🟡 Medium |
  ```

  For each individual change, assess risk by considering:
  - Does it alter runtime behavior on AKS nodes?
  - Does it change CLI flags, config file formats, or systemd unit behavior that CSE scripts depend on?
  - Does it affect GPU workloads, networking, container runtime, or kubelet interaction?
  - Could it increase binary size significantly (VHD bloat)?
  - Does it introduce new system dependencies or kernel requirements?

  **If upstream changelog is unavailable**, explicitly state: _"Upstream changelog not found for this version range. Manual testing recommended before merge."_

  **Overall risk assessment:**

  ```
  ### Overall Risk: 🟢 Low / 🟡 Medium / 🔴 High
  **Justification**: <1-2 sentence summary of why this risk level was chosen>
  **Recommendation**: Approve / Request more info / Flag for manual testing
  ```

## Analysis Approach

**Dynamic Dependency Tracing**:

1. For each changed file, identify what depends on it
2. Follow `source` statements in bash scripts to trace dependency chains
3. Check for function calls, variable references across files
4. Look for hardcoded paths in VHD build scripts (`vhdbuilder/packer/`) that reference changed files
5. Trace through as many levels as needed within the codebase
6. **Check external dependencies**:
   - Search for new URLs being downloaded (curl, wget, etc.)
   - Verify all external dependencies are in `parts/common/components.json` for Renovate updates
   - Flag downloads from unauthorized sources (only packages.aks.azure.com and sources in components.json allowed)

**Historical Context**:

- Look for related changes that previously caused issues
- Identify patterns of fragile areas that break frequently

**Test Coverage Assessment**:

- Assess whether tests cover the changed behavior.
- Report missing coverage only when a concrete behavior or failure mode has no effective validation.

## Review Output Format

Report only substantive findings that the changed code supports. Each finding must identify a concrete failure mode.

For each finding:

- Comment directly on the problematic line or code block
- Explain the failure and the affected configurations or lifecycle stages
- Suggest specific mitigations or alternatives

**Risk indicators to include:**

- **Severity** (pick one):
  - 🔴 **High Risk** — Could break production VM provisioning, cause node failures, or introduce security vulnerabilities
  - 🟡 **Medium Risk** — Could cause issues in specific configurations, edge cases, or degrade performance

- **Category** (pick one):
  - 🔧 **Script Logic** — Syntax errors, incorrect commands, broken control flow, wrong exit codes
  - 🖥️ **Cross-OS** — Incompatibility between Ubuntu, Azure Linux/Mariner, or Windows
  - 🌐 **External Dependency** — Unauthorized downloads, missing components.json entries, broken URLs
  - 🧪 **Test Coverage** — Missing or insufficient test coverage for changed behavior
  - 📦 **Package Update** — Component version changes, upstream regressions, VHD size impact
  - 🔄 **Backward Compatibility** — Breaking changes affecting VHDs in production (6-month window)
  - 🔒 **Security** — Credential exposure, privilege escalation, insecure defaults
  - ⚡ **Performance** — VHD build time regression, node provisioning latency increase
  - 🏗️ **Architecture** — Structural changes affecting multiple components or deployment modes

If there are no substantive findings, return no findings.

## Review Philosophy

- Understand the architecture and how components interact
- Consider timing of releases and deployment sequences
- Reason about implicit dependencies and assumptions
- Balance thoroughness with actionable feedback
- Focus on high-impact issues that could break production VM provisioning