devops-lib-gcp/review-learnings.md

# PR Review Guide

> Derived from human review discussions across 26 PRs merged into `main` (PRs #306–#727).
> Focus: P0 (production-impacting) and P1 (bugs, build reliability, architecture, error handling, observability) signals.
> Excluded: Style, formatting, refactors, lint-level feedback.

---

## P0 — Must Not Miss (Production Impact)

```yaml
- id: P0_DEBUG_URL_IN_PRODUCTION_CONFIG
  severity: P0
  title: Debug/tunnel URLs must not reach production pipeline code
  trigger_paths:
    - "src/com/meesho/stages/*.groovy"
    - "src/com/meesho/utilities/*.groovy"
  required_evidence:
    - "No lhr.life, ngrok, localtunnel, or similar tunnel hostnames present in any switch/case or URL assignment"
    - "All cicdBaseUrl values point to stable internal DNS names (e.g. turbo-turtle.meeshogcp.in)"
  suggested_fix:
    - "Revert any lhr.life or debug tunnel URL to the canonical internal DNS before merging"
    - "Add a pre-commit or CI grep that fails on *.lhr.life, *.ngrok.io, *.tunnel.* patterns in .groovy files"
  why: >
    PRs #596 and #664 both accidentally merged lhr.life tunnel URLs into deployRingmaster.groovy,
    replacing the production Turbo-Turtle endpoint. This would break all production and int-environment
    deployments silently — pipelines would attempt callbacks to an ephemeral tunnel that no longer exists.
  references:
    - "PR #596"
    - "PR #664"

- id: P0_SENSITIVE_DEBUG_PRINT_IN_PIPELINE
  severity: P0
  title: Pipeline code must not print SSH private keys or credentials to console
  trigger_paths:
    - "src/com/meesho/utilities/addSSHKey.groovy"
    - "src/com/meesho/**/*.groovy"
    - "resources/com/meesho/*Dockerfile"
  required_evidence:
    - "No `cat`, `echo`, or `print` of private key file contents (id_github_jenkins, id_rsa, etc.)"
    - "No `ls -al` of credential file paths in shipped pipeline code"
  suggested_fix:
    - "Remove any `cat ./id_github_jenkins` or `ls -al <key-path>` lines before merging"
    - "Debug prints of credentials are acceptable only in local branches; must be stripped before PR"
  why: >
    PR #634 shipped `cat ./id_github_jenkins` inside addSSHKey.groovy. Jenkins build logs are accessible
    to all users with job read access — printing private key content exposes the key to anyone who can
    view the console output, which is a critical credential leak.
  references:
    - "PR #634"

- id: P0_HARDCODED_IP_IN_PIPELINE
  severity: P0
  title: Internal service endpoints must use DNS names, not hardcoded IPs
  trigger_paths:
    - "src/com/meesho/stages/*.groovy"
    - "src/com/meesho/utilities/*.groovy"
  required_evidence:
    - "No bare IP addresses (e.g. 172.x.x.x, 10.x.x.x) used as curl/HTTP targets in pipeline code (known exception: securityScan.groovy:11 uses 172.31.5.29:63232 pending remediation)"
    - "All internal service calls use DNS-resolvable hostnames"
  suggested_fix:
    - "Replace IP literals with DNS names before merging"
    - "If no DNS exists yet, create one before the PR lands — do not ship with the IP as a placeholder"
  why: >
    PR #681 hardcoded `http://172.23.72.116:5002` as the build-callback endpoint in notify.groovy.
    IP addresses break silently when infra is rebalanced or services are migrated, with no indication
    in the pipeline code that the endpoint has moved.
  references:
    - "PR #681"

- id: P0_CONFIG_TEMPLATE_INJECTION_SAFETY
  severity: P0
  title: User-supplied config data embedded in YAML templates must be sanitized
  trigger_paths:
    - "resources/com/meesho/values.yaml"
    - "src/com/meesho/stages/deployArgoCD.groovy"
  required_evidence:
    - "Any `.trim().replaceAll(...)` applied to externally-sourced config data is reviewed for correctness"
    - "Indentation injection via `replaceAll(\"(?m)^\", \" \")` is validated against multiline edge cases"
    - "User-controlled strings containing YAML special characters (`:`, `|`, `>`, `#`) are escaped or block-quoted"
  suggested_fix:
    - "Apply strict input validation or schema-based parsing before interpolating user config into YAML templates"
    - "Prefer a YAML library for construction over string interpolation"
  why: >
    PR #343 reviewer flagged that 'people can put strange stuff in their configs' when raw config data
    is string-interpolated into the values.yaml template. Malformed or adversarial input can corrupt
    the YAML structure silently, causing ArgoCD deployments to use wrong or empty config.
  references:
    - "PR #343"
```

---

## P1 — Important Improvements (Reliability, Build Health, Architecture)

```yaml
- id: P1_EXTERNAL_BINARY_DOWNLOAD_IN_BUILD
  severity: P1
  title: External binaries must not be downloaded on every pipeline run
  trigger_paths:
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildNode.groovy"
    - "resources/com/meesho/*Dockerfile"
  required_evidence:
    - "No `curl -LO https://binaries.sonarsource.com/...` or `curl -LO https://go.dev/dl/...` inside the stage body"
    - "Sonar-scanner binary is baked into the relevant Docker image or fetched from internal Artifactory"
    - "Go toolchain is fetched from Artifactory (not go.dev) when a non-image version is needed"
  suggested_fix:
    - "Bake sonar-scanner into the builder base image; reference it as a path, not a curl download"
    - "For Go version management, pull tarballs from internal Artifactory instead of go.dev to avoid external dependency and improve speed"
    - "If dynamic version selection is needed, cache the download to a Jenkins agent workspace layer"
  why: >
    PR #643 added `curl -LO https://binaries.sonarsource.com/.../sonar-scanner-cli-5.0.1.3006-linux.zip`
    and `curl -LO https://go.dev/dl/go${goVersion}.linux-amd64.tar.gz` inside the build stage body.
    This increases build time on every run, creates a hard dependency on external internet connectivity,
    and makes pipelines brittle when upstream URLs change or are rate-limited.
  references:
    - "PR #643"

- id: P1_MISSING_PREREQUISITE_FILE_CHECK
  severity: P1
  title: Required config files must be verified to exist before stages that depend on them
  trigger_paths:
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildNode.groovy"
  required_evidence:
    - "If a stage uses `sonar-project.properties`, `pom.xml`, or equivalent config, existence is checked before the stage runs"
    - "Missing file results in a clear failure message, not a cryptic scanner error mid-stage"
  suggested_fix:
    - "Add `if (!fileExists('sonar-project.properties')) { error('sonar-project.properties not found — cannot run Sonar scan') }` before the scan stage"
  why: >
    PR #643 reviewer flagged that if sonar-project.properties does not exist, the scan stage fails with
    a cryptic error mid-execution. An upfront existence check surfaces the real problem immediately and
    avoids partial stage execution that leaves the pipeline in an ambiguous state.
  references:
    - "PR #643"

- id: P1_TEST_FAILURE_MUST_FAIL_PIPELINE
  severity: P1
  title: Test failures must cause the build to fail, not silently continue
  trigger_paths:
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildNode.groovy"
  required_evidence:
    - "Non-zero test exit codes propagate as build failures or are explicitly documented as intentional with a tracking ticket"
    - "Any `|| true` or equivalent suppression on a test command has a comment explaining why"
  suggested_fix:
    - "Remove `if (testExitCode != 0) { sh('echo tests failed, pipeline continues') }` patterns"
    - "If tests are intentionally non-blocking, capture the result as a warning in the build report rather than silently absorbing the failure"
  why: >
    PR #643 had a pattern where Go test failures were caught, logged as 'pipeline will continue', and
    execution proceeded to Sonar and Docker stages. This allows broken code to be packaged and deployed
    while appearing to pass CI, defeating the purpose of test gating.
  references:
    - "PR #643"

- id: P1_CENTRALIZED_CLEANUP_IN_FINALLY
  severity: P1
  title: Build cleanup logic must be in a centralized finally block, not scattered try-catches
  trigger_paths:
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildNode.groovy"
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildRust.groovy"
  required_evidence:
    - "New failure/cleanup paths are handled via a shared cleanup function called from the build's finally block"
    - "Cleanup is not added only to the specific branch/case that was fixed — it covers all known failure scenarios"
  suggested_fix:
    - "Extract a `cleanupOnFailure(config)` function in each build stage file"
    - "Call it from the `finally` block of the top-level try/catch in buildMaven, buildNode, buildGo, buildRust"
  why: >
    PR #533 reviewer requested that cleanup on failed Docker tag operations be moved to the overall
    `finally` block rather than an ad-hoc inner try-catch. Scattered cleanup means new failure modes
    (aborts, timeouts, new stages) bypass cleanup silently — a centralized `finally` ensures cleanup
    is language-agnostic and future-proof.
  references:
    - "PR #533"

- id: P1_SEMANTIC_FLAG_REUSE
  severity: P1
  title: Pipeline control flags must have precise semantics; do not reuse existing flags for new meanings
  trigger_paths:
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildNode.groovy"
    - "src/com/meesho/stages/hotFix.groovy"
  required_evidence:
    - "New pipeline behaviors controlled by a new, clearly named env flag (not piggybacking on `hot_fix`, `skip_sonar`, etc.)"
    - "The flag name matches the intent: `revert_pr`, `skip_quality_gate`, `is_config_only_change`"
  suggested_fix:
    - "Introduce `env.revert_pr` rather than overloading `env.hot_fix` for revert PR detection"
    - "Document new flags in the devops-lib README or pipeline configuration guide"
  why: >
    PR #578 used `env.hot_fix` to skip quality gates for revert PRs, which a reviewer flagged as a
    semantic hack. PR #495 added a redundant `env.hot_fix || skip_sonar` check when `skip_sonar` was
    already sufficient. Overloading flags creates confusion about which paths are active and makes
    future changes to either flag risky.
  references:
    - "PR #578"
    - "PR #495"

- id: P1_DOCKER_LAYER_CACHING_DEPENDENCY_FIRST
  severity: P1
  title: Dockerfiles must copy dependency manifests before source to enable layer caching
  trigger_paths:
    - "resources/com/meesho/*Dockerfile"
    - "resources/com/meesho/rust-Dockerfile"
  required_evidence:
    - "Dependency manifest files (Cargo.toml/Cargo.lock, package.json/package-lock.yaml, go.mod/go.sum, pom.xml) are COPY'd and dependencies fetched before COPY . ."
    - "Source-only changes do not invalidate the dependency install layer"
  suggested_fix:
    - "For Rust: `COPY Cargo.toml Cargo.lock ./` → `RUN cargo fetch` → `COPY . .` → `RUN cargo build --release`"
    - "For Node: `COPY package.json package-lock.yaml ./` → `RUN npm ci` → `COPY . .` → `RUN npm run build`"
  why: >
    PR #650 reviewer flagged that the Rust Dockerfile did `COPY . .` before fetching dependencies,
    meaning every source change triggers a full `cargo build` including all dependency compilation.
    Splitting the copy invalidates the dependency layer only on manifest changes, dramatically reducing
    incremental build times.
  references:
    - "PR #650"

- id: P1_HARDCODED_TOOL_VERSION_IN_DOCKERFILE
  severity: P1
  title: Base image tool versions must be developer-configurable, not hardcoded in Dockerfiles
  trigger_paths:
    - "resources/com/meesho/*Dockerfile"
    - "resources/com/meesho/rust-Dockerfile"
  required_evidence:
    - "Runtime/language version (Rust, Go, Java, Node) is parameterized as a build ARG or resolved from the service config, not hardcoded"
    - "Pattern is consistent with how Go, Java, and Python versions are handled in existing Dockerfiles"
  suggested_fix:
    - "Use `ARG RUST_VERSION=1.86.0` and `FROM .../rust:${RUST_VERSION} AS builder` — pass the version from the pipeline config"
  why: >
    PR #650 hardcoded `FROM .../rust:1.86.0` in the Rust Dockerfile. Reviewer noted this is inconsistent
    with Go, Java, and Python where the version is developer-configurable via the service config. Services
    needing a different Rust version would require a devops-lib PR rather than a self-service config change.
  references:
    - "PR #650"

- id: P1_DYNAMIC_DOCKERFILE_MUTATION_VIA_SHELL
  severity: P1
  title: Dockerfiles must not be constructed by appending lines via shell echo at build time
  trigger_paths:
    - "src/com/meesho/stages/buildNode.groovy"
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildMaven.groovy"
  required_evidence:
    - "No `sh \"echo '...' >> Dockerfile-${artifactId}\"` patterns in build stage code"
    - "Conditional Dockerfile content is handled via ARG/build-arg, multi-stage targets, or separate Dockerfile templates"
  suggested_fix:
    - "Use `ARG USE_CAC_PATH` in the Dockerfile template and conditional `RUN` blocks based on that arg"
    - "If the feature is truly mode-specific, maintain a separate Dockerfile template (e.g., `node-cac-Dockerfile`) rather than mutating a shared one at runtime"
  why: >
    PR #707 appended `RUN truncate -s 0 .env` to the Dockerfile via `sh echo >>` when CAC was enabled.
    Runtime Dockerfile mutation makes the actual image instructions invisible in code review, untestable
    in isolation, and fragile to ordering assumptions.
  references:
    - "PR #707"

- id: P1_FEATURE_BRANCH_IN_ENVIRONMENT_MAP
  severity: P1
  title: Feature branch names must not be hardcoded in the environment/branch mapping tables
  trigger_paths:
    - "src/com/meesho/utilities/constructParam.groovy"
    - "src/com/meesho/stages/deployArgoCD.groovy"
  required_evidence:
    - "The `environment_map` and `branch_param_map` in constructParam.groovy / deployArgoCD.groovy contain only permanent branch names (master, main, develop, gcp-main, etc.)"
    - "No `feature/...` branch names are present in these maps"
  suggested_fix:
    - "Remove test-only feature branch entries before merging"
    - "If special branch routing is needed for integration testing, use a mechanism that is not committed to the shared library (e.g., a per-repo override config)"
  why: >
    PR #707 added `feature/config_as_code_main` to both environment_map and branch_param_map as a
    testing convenience. Reviewer questioned the purpose and the author confirmed it was test-only.
    Leaving it in the shared library would mean all repos inheriting devops-lib would attempt to route
    this branch to staging, which is incorrect.
  references:
    - "PR #707"

- id: P1_SCRIPT_REUSE_OVER_DUPLICATION
  severity: P1
  title: Config validation scripts must be shared, not duplicated per language
  trigger_paths:
    - "resources/com/meesho/validate_configs*.py"
  required_evidence:
    - "New language-specific validation needs extend `validate_configs_v2.py` (or the current canonical script) rather than creating a new file"
    - "Duplication is justified only when the new script has fundamentally different logic that cannot be parameterized"
  suggested_fix:
    - "Extend `validate_configs_v2.py` with language-specific config schemas rather than copying it to `validate_configs_node.py`"
    - "Use a `--lang` argument or config-driven schema selection to handle per-language differences"
  why: >
    PR #707 introduced `validate_configs_node.py` (470 lines) when `validate_configs_v2.py` already
    existed. Duplicating the validation script means bug fixes and new validation rules must be applied
    in multiple places, and the scripts will drift over time.
  references:
    - "PR #707"

- id: P1_TOOLS_IN_BASE_IMAGE_NOT_BUILD_STEP
  severity: P1
  title: Globally-used tools must be baked into the base image, not installed per build
  trigger_paths:
    - "resources/com/meesho/*Dockerfile"
    - "src/com/meesho/stages/*.groovy"
  required_evidence:
    - "No `npm install -g <tool>` or `apt-get install <tool>` inside the Docker build RUN steps that are also used globally"
    - "Tools like sonar-scanner, pnpm, and librdkafka are in the base image where they apply to all builds"
  suggested_fix:
    - "Move `npm install -g sonar-scanner` and similar global installs to the base Node image build process"
    - "Cut a new base image version that includes the tool rather than installing it on each application build"
  why: >
    PR #306 flagged `npm install -g sonar-scanner` in the application Dockerfile — this runs on every
    build for every service. Moving it to the base image eliminates the per-build install cost and
    ensures all services use the same tool version.
  references:
    - "PR #306"

- id: P1_DOWNLOADED_BINARY_HASH_VERIFICATION
  severity: P1
  title: Externally downloaded binaries must have their integrity verified
  trigger_paths:
    - "src/com/meesho/stages/buildGo.groovy"
    - "resources/com/meesho/*Dockerfile"
  required_evidence:
    - "Any `curl -LO` downloading a binary is followed by a hash/checksum verification step"
    - "Or the download is replaced by an Artifactory-hosted artifact with controlled provenance"
  suggested_fix:
    - "After downloading, run `sha256sum -c <expected-hash>` or use the official checksum file provided by the vendor"
    - "Prefer Artifactory-hosted binaries (Meesho-controlled) to eliminate the need for runtime checksum verification"
  why: >
    PR #643 reviewer requested: 'Could we also verify that the downloaded artifact matches its computed hash?'
    Pipelines downloading binaries from the internet without verification are vulnerable to supply-chain
    attacks if the upstream URL is compromised or served a modified artifact.
  references:
    - "PR #643"

- id: P1_WRONG_ENV_VAR_IN_CONTAINER
  severity: P1
  title: Container environment variables must be appropriate for the runtime language
  trigger_paths:
    - "resources/com/meesho/rust-values.yaml"
    - "resources/com/meesho/rust-Dockerfile"
    - "resources/com/meesho/*-values.yaml"
  required_evidence:
    - "GOMAXPROCS is not set in non-Go containers (Rust, Java, Node, Python)"
    - "Any env vars carried over from a template are validated as relevant to the target runtime"
  suggested_fix:
    - "Remove `GOMAXPROCS` from rust-values.yaml — it is a Go runtime tuning parameter and has no effect in a Rust binary"
    - "When creating a new language template by copying an existing one, audit all env vars for relevance"
  why: >
    PR #650 set `GOMAXPROCS` in the Rust service values.yaml template, which a reviewer questioned.
    This is a Go-specific knob that controls goroutine scheduling and is meaningless in a Rust process.
    Beyond being confusing, copying env vars blindly from templates creates operational noise and
    incorrect assumptions for future operators.
  references:
    - "PR #650"

- id: P1_SHARED_CONSTANTS_FOR_REGISTRY_PATHS
  severity: P1
  title: Repeated infrastructure constants (registry URLs, base paths) must be defined as shared variables
  trigger_paths:
    - "src/com/meesho/stages/*.groovy"
    - "src/com/meesho/utilities/*.groovy"
  required_evidence:
    - "Registry base URLs like `asia-southeast1-docker.pkg.dev/meesho-central-dev-0622/toolchain` appear in a single shared constant, not repeated inline across stage files"
    - "New stage files reference the shared constant rather than re-declaring the string"
  suggested_fix:
    - "Define `TOOLCHAIN_REGISTRY_BASE` (or equivalent) in a shared constants file/class in `src/com/meesho/utilities/`"
    - "Reference it in notify.groovy and any other stage that constructs toolchain image paths"
  why: >
    PR #681 reviewer flagged `def base_registry = \"asia-southeast1-docker.pkg.dev/meesho-central-dev-0622/toolchain\"`
    as an inline constant that should be a shared variable. Duplicating registry paths means a project
    or region migration requires finding and updating every occurrence individually.
  references:
    - "PR #681"
```

---

## Common Failure Patterns Observed

```yaml
- id: PATTERN_DEBUG_ARTIFACT_LEFT_IN_PRODUCTION_CODE
  title: Debug artifacts and test configurations merged into main pipeline code
  description: >
    Multiple PRs merged temporary debug configurations (tunnel URLs, hardcoded IPs, feature branch
    mappings, SSH key prints, debug echo statements) that were introduced during local testing and
    not cleaned up before merge. The pattern appears in deployRingmaster.groovy (lhr.life URLs),
    notify.groovy (IP address), constructParam.groovy (feature branch mapping), and addSSHKey.groovy
    (credential printing). The shared library nature of devops-lib means these reach all services
    immediately on merge.
  related_rules:
    - "P0_DEBUG_URL_IN_PRODUCTION_CONFIG"
    - "P0_SENSITIVE_DEBUG_PRINT_IN_PIPELINE"
    - "P0_HARDCODED_IP_IN_PIPELINE"
    - "P1_FEATURE_BRANCH_IN_ENVIRONMENT_MAP"
  references:
    - "PR #596"
    - "PR #634"
    - "PR #664"
    - "PR #681"
    - "PR #707"

- id: PATTERN_PER_BUILD_TOOL_INSTALLATION
  title: Tools that should be in base images are installed on every build run
  description: >
    Several PRs introduced per-build installation of tools that belong in the builder base image:
    sonar-scanner downloaded via curl on every Go and Node build, Go toolchain downloaded from go.dev
    on every Sonar scan, librdkafka and pnpm installed in application Dockerfiles. This pattern slows
    all builds, adds external network dependencies to the critical path, and creates version drift
    when different builds download different patch versions.
  related_rules:
    - "P1_EXTERNAL_BINARY_DOWNLOAD_IN_BUILD"
    - "P1_TOOLS_IN_BASE_IMAGE_NOT_BUILD_STEP"
    - "P1_DOWNLOADED_BINARY_HASH_VERIFICATION"
  references:
    - "PR #306"
    - "PR #643"
    - "PR #650"

- id: PATTERN_COPY_PASTE_WITHOUT_AUDIT
  title: New language support created by copying existing templates without auditing all fields
  description: >
    When adding Rust CICD support (PR #650) and Node CAC support (PR #707), reviewers flagged that
    values.yaml templates, Dockerfiles, and validation scripts were copied from an existing language
    without removing inapplicable settings (GOMAXPROCS in Rust) or reusing shared logic
    (validate_configs_node.py vs validate_configs_v2.py). Copy-paste propagates bugs and misconfigurations
    from the source template to the new language.
  related_rules:
    - "P1_WRONG_ENV_VAR_IN_CONTAINER"
    - "P1_SCRIPT_REUSE_OVER_DUPLICATION"
    - "P1_HARDCODED_TOOL_VERSION_IN_DOCKERFILE"
    - "P1_DOCKER_LAYER_CACHING_DEPENDENCY_FIRST"
  references:
    - "PR #650"
    - "PR #707"
```

---

## Review Checklist

Before approving, verify:

**Security & Credentials**
- [ ] **No debug/tunnel URLs** (P0_DEBUG_URL_IN_PRODUCTION_CONFIG): No lhr.life, ngrok, or localtunnel hostnames in any `.groovy` or config file
- [ ] **No credential printing** (P0_SENSITIVE_DEBUG_PRINT_IN_PIPELINE): No `cat`, `echo`, or `print` of SSH key or token file contents
- [ ] **No hardcoded IPs** (P0_HARDCODED_IP_IN_PIPELINE): All service endpoints use DNS names, not bare IP literals

**Config & Template Safety**
- [ ] **Config injection safety** (P0_CONFIG_TEMPLATE_INJECTION_SAFETY): User-supplied data embedded in YAML templates is sanitized against multiline and special-character edge cases
- [ ] **No feature branches in env maps** (P1_FEATURE_BRANCH_IN_ENVIRONMENT_MAP): `environment_map` and `branch_param_map` contain only permanent branch names

**Build Reliability**
- [ ] **No per-build tool downloads** (P1_EXTERNAL_BINARY_DOWNLOAD_IN_BUILD): sonar-scanner, Go tarball, or other tools are not curl-downloaded inside the stage body
- [ ] **Binary integrity** (P1_DOWNLOADED_BINARY_HASH_VERIFICATION): Any downloaded binary has a hash/checksum verification step, or comes from Artifactory
- [ ] **Prerequisite file checks** (P1_MISSING_PREREQUISITE_FILE_CHECK): Required config files (sonar-project.properties, etc.) are checked for existence before stages that depend on them
- [ ] **Test failures fail the build** (P1_TEST_FAILURE_MUST_FAIL_PIPELINE): Non-zero test exit codes propagate as pipeline failures; no silent suppression
- [ ] **Tools in base image** (P1_TOOLS_IN_BASE_IMAGE_NOT_BUILD_STEP): Global tools (sonar-scanner, pnpm, librdkafka) are in the base image, not installed per-build

**Docker & Image Quality**
- [ ] **Dependency-first Dockerfile layering** (P1_DOCKER_LAYER_CACHING_DEPENDENCY_FIRST): Manifests (Cargo.toml, package.json) are copied and deps fetched before `COPY . .`
- [ ] **Parameterized tool versions** (P1_HARDCODED_TOOL_VERSION_IN_DOCKERFILE): Language/runtime version is a build ARG, not hardcoded in FROM
- [ ] **No runtime Dockerfile mutation** (P1_DYNAMIC_DOCKERFILE_MUTATION_VIA_SHELL): No `echo '...' >> Dockerfile-*` patterns in pipeline code
- [ ] **Correct env vars for runtime** (P1_WRONG_ENV_VAR_IN_CONTAINER): No Go-specific vars (GOMAXPROCS) in non-Go containers; template env vars audited for relevance

**Code Architecture**
- [ ] **Centralized cleanup** (P1_CENTRALIZED_CLEANUP_IN_FINALLY): New failure paths route through the shared `finally` cleanup function, not ad-hoc inner try-catches
- [ ] **Precise flag semantics** (P1_SEMANTIC_FLAG_REUSE): New pipeline behaviors use new, clearly named flags; not piggybacking on `hot_fix` or `skip_sonar`
- [ ] **Shared constants** (P1_SHARED_CONSTANTS_FOR_REGISTRY_PATHS): Registry URLs and infra paths are defined as shared constants, not repeated inline
- [ ] **Script reuse** (P1_SCRIPT_REUSE_OVER_DUPLICATION): Config validation logic extends the existing canonical script; no new duplicate validation scripts

---

## CodeRabbit-Sourced Learnings

> Derived from CodeRabbit automated review comments across PRs #664, #727, and related.
> Merged into this file per AI Blitz Task 10.

```yaml
- id: CR_UNDEFINED_VARIABLE_IN_NEW_STAGE
  severity: P0
  title: New config fields must be defined in every build stage that references them
  trigger_paths:
    - "src/com/meesho/stages/buildRust.groovy"
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildNode.groovy"
    - "src/com/meesho/stages/buildGradle.groovy"
  required_evidence:
    - "Every variable used in conditional logic (e.g. `repoType`, `skip_npmrc`) is extracted from `config` with a default value before first use"
    - "Compare with existing build stages to ensure parity: if buildGo defines `def repoType = config.repoType ?: 'unknown'`, buildRust must too"
  suggested_fix:
    - "Add `def repoType = config.repoType ?: 'unknown'` near the other config extractions in the function"
  why: >
    CodeRabbit flagged in PR #664 that `repoType` was used in buildRust.groovy
    without being defined, causing a MissingPropertyException at runtime. The variable
    was properly defined in buildGo, buildGradle, and buildMaven but missed in the
    new Rust stage.
  references:
    - "PR #664 (CodeRabbit)"

- id: CR_INVERTED_CONDITIONAL_ON_REPOTYPE
  severity: P0
  title: Docker build conditionals on repoType must use == not != for the intended type
  trigger_paths:
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildNode.groovy"
    - "src/com/meesho/stages/buildGradle.groovy"
    - "src/com/meesho/stages/buildRust.groovy"
  required_evidence:
    - "Conditionals guarding Docker build (`stageDocker`) use `repoType == 'microservice'` (not `!=`)"
    - "Both the GCP and fallback/AWS branches have the same guard polarity"
  suggested_fix:
    - "Replace `repoType != 'microservice'` with `repoType == 'microservice'` in the stageDocker conditional"
  why: >
    CodeRabbit caught in PR #664 that `repoType != 'microservice'` caused Docker
    builds to run for library/client repos and skip for microservices — the exact
    opposite of the PR's intent. The same inversion appeared in both the GCP and
    fallback code paths.
  references:
    - "PR #664 (CodeRabbit)"

- id: CR_GROOVY_SINGLE_QUOTE_NO_INTERPOLATION
  severity: P1
  title: Log messages with variable references must use double quotes for GString interpolation
  trigger_paths:
    - "src/com/meesho/stages/*.groovy"
    - "src/com/meesho/utilities/*.groovy"
  required_evidence:
    - "No log.info/log.warn/log.error calls use single-quoted strings containing `${...}` placeholders"
    - "All strings with variable interpolation use double quotes (`\"...\"`)"
  suggested_fix:
    - "Change `log.info('Skipping for ${repoType}')` to `log.info(\"Skipping for ${repoType}\")`"
  why: >
    CodeRabbit flagged in PR #664 that single-quoted Groovy strings do NOT support
    interpolation — `'${repoType}'` prints the literal text `${repoType}`, not its value.
    Double-quoted strings become GStrings and evaluate `${}` expressions.
  references:
    - "PR #664 (CodeRabbit)"

- id: CR_MISLEADING_SKIP_LOG_MESSAGE
  severity: P1
  title: Skip/bypass log messages must state the actual reason for skipping
  trigger_paths:
    - "src/com/meesho/stages/buildGo.groovy"
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildNode.groovy"
  required_evidence:
    - "Log messages in else/skip branches mention the actual condition that triggered the skip (e.g. `shouldValidateConfig=false` or `CHANGE_ID not set`)"
    - "Messages do not blame `repoType` when the skip is caused by a different condition"
  suggested_fix:
    - "Include the actual condition values: `log.info(\"Skipping CAC validation (repoType=${repoType}, changeId=${env.CHANGE_ID ?: 'N/A'})\")`"
  why: >
    CodeRabbit flagged in PR #664 that the else branch logged "Skipping for repoType"
    when the actual reason was that shouldValidateConfig returned false or CHANGE_ID
    was not set. Misleading skip messages make debugging pipeline behavior harder.
  references:
    - "PR #664 (CodeRabbit)"

- id: CR_DEFAULT_FLAG_BREAKS_EXISTING_BEHAVIOR
  severity: P1
  title: New boolean config flags must default to preserving existing behavior
  trigger_paths:
    - "src/com/meesho/stages/buildNode.groovy"
    - "src/com/meesho/stages/buildMaven.groovy"
    - "src/com/meesho/stages/buildGo.groovy"
  required_evidence:
    - "New flags (e.g. `skip_npmrc`, `keep_package_lock`) default to the pre-existing behavior when not explicitly set"
    - "If existing behavior is to include `.npmrc`, the default must be `skip_npmrc: false`, not `true`"
  suggested_fix:
    - "Change `def skip_npmrc = config.skip_npmrc != null ? config.skip_npmrc : true` to default `false` to preserve existing behavior"
  why: >
    CodeRabbit flagged in PR #727 that defaulting skip_npmrc to true would break
    repos relying on private registries that need the secret-managed .npmrc. The
    shared library serves all services — a new opt-in flag must not change default
    behavior for existing consumers.
  references:
    - "PR #727 (CodeRabbit)"

- id: CR_PACKAGE_LOCK_REMOVAL_VS_NPM_CI
  severity: P1
  title: Removing package-lock.json is incompatible with npm ci as the default install command
  trigger_paths:
    - "src/com/meesho/stages/buildNode.groovy"
    - "resources/com/meesho/node-Dockerfile"
  required_evidence:
    - "If `keep_package_lock=false` removes the lock file, `npm_install_arg` must not be empty (which defaults to `npm ci` in the Dockerfile)"
    - "The Dockerfile fallback install command and the Groovy-side lock file removal logic are in sync"
  suggested_fix:
    - "When `keep_package_lock=false` and `npm_install_arg` is empty, set `npm_install_arg = 'npm install'` to avoid `npm ci` failing on missing lockfile"
  why: >
    CodeRabbit flagged in PR #727 that removing package-lock.json while leaving
    npm_install_arg empty causes the Dockerfile to fall back to `npm ci`, which
    requires a lockfile and will fail. The Groovy stage and Dockerfile must agree
    on the install strategy.
  references:
    - "PR #727 (CodeRabbit)"
```

### CodeRabbit Review Checklist Additions

Before approving, also verify:

**Variable & Conditional Correctness (CodeRabbit-sourced)**
- [ ] **No undefined config variables** (CR_UNDEFINED_VARIABLE_IN_NEW_STAGE): Every variable used in a build stage is extracted from `config` with a default before first use
- [ ] **Correct conditional polarity** (CR_INVERTED_CONDITIONAL_ON_REPOTYPE): Docker build guards use `==` for the intended type, not `!=`
- [ ] **Double quotes for interpolation** (CR_GROOVY_SINGLE_QUOTE_NO_INTERPOLATION): No `log.info('${var}')` — must be `log.info("${var}")`
- [ ] **Accurate skip messages** (CR_MISLEADING_SKIP_LOG_MESSAGE): Skip/bypass logs state the actual condition, not a misleading one
- [ ] **Safe flag defaults** (CR_DEFAULT_FLAG_BREAKS_EXISTING_BEHAVIOR): New boolean config flags default to preserving pre-existing behavior
- [ ] **Lock file vs install command sync** (CR_PACKAGE_LOCK_REMOVAL_VS_NPM_CI): Removing lock files is paired with a compatible install command