skills: codify GCP authoring lessons from CodeBundle live-run reviews

skills/auth-gcp.md

@@ -124,6 +124,48 @@ Apply this pattern to **both** `runbook.robot` and `sli.robot`.
 
 ---
 
+## Cross-Project Quota Project (the SERVICE_DISABLED trap)
+
+When the credential's own project differs from the target project --
+i.e. a Workload-Identity or service-account whose home project is *not*
+the `GCP_PROJECT_ID` you're monitoring -- gcloud derives the API
+consumer / quota project from the **credential's** project, not from
+`--project` / `CLOUDSDK_CORE_PROJECT`. Any API that checks its own
+enablement (Service Usage, Cloud Monitoring, Cloud Spanner, ...) then
+fails against the *caller's* project with:
+
+```text
+PERMISSION_DENIED: Cloud Spanner API has not been used in project
+<caller-project> before or it is disabled ... SERVICE_DISABLED
+```
+
+even though the API is perfectly enabled on the target. The check is
+reading the wrong project.
+
+**Fix: pin the quota/billing project to the target in the suite env.**
+
+```robot
+Set Suite Variable
+...    ${env}
+...    {"CLOUDSDK_CORE_PROJECT":"${GCP_PROJECT_ID}","CLOUDSDK_BILLING_QUOTA_PROJECT":"${GCP_PROJECT_ID}","GOOGLE_APPLICATION_CREDENTIALS":"./${gcp_credentials.key}","PATH":"$PATH:${OS_PATH}","GCP_PROJECT_ID":"${GCP_PROJECT_ID}"}
+```
+
+`CLOUDSDK_BILLING_QUOTA_PROJECT` (equivalently `gcloud config set
+billing/quota_project`) forces gcloud and REST calls made through the
+gcloud session to bill/consume against `${GCP_PROJECT_ID}`.
+
+- **Cross-project SAs / Workload Identity** need
+  `roles/serviceusage.serviceUsageConsumer` on the **target** project
+  for the pinned quota project to be usable.
+- **In-project SAs** (credential home == target) are unaffected --
+  setting it is a **no-op**, so it is always safe to include.
+
+Set it in **both** `runbook.robot` and `sli.robot` suite env. This bites
+every GCP bundle used with WI or a shared/cross-project SA; add it by
+default.
+
+---
+
 ## Runtime Environment (what the platform sets for you)
 
 `runrobot.py` prepares these before Robot starts. Do **not** override
@@ -223,8 +265,16 @@ Rules:
    (`bigquery.tables.list`, `bigquery.routines.list`) that read-only
    service accounts often lack; `bq ls`/`bq show` work with basic
    viewer roles.
-4. **Degrade gracefully**: `2>/dev/null || echo "[]"` on discovery
-   commands so a permission gap produces an empty result, not a crash.
+4. **Degrade gracefully -- but only for commands that exist.**
+   `2>/dev/null || echo "[]"` is fine on a `gcloud`/`bq` *discovery*
+   command where the only expected failure is a **permission gap** (turn
+   `Access Denied` into an empty result, not a crash). **Never** wrap it
+   around a command whose existence or success isn't guaranteed -- a
+   typo'd subcommand, a REST call, an unsupported flag. That pattern
+   converts "command not found" / "API error" into "no data", which a
+   health check then scores as passing. For calls that can fail that way,
+   **validate the response shape** and **fail loud** instead of
+   defaulting to `[]`.
 5. **Check all BigQuery access field variants** -- `bq show` returns
    public principals under `specialGroup`, `iamMember`, or
    `groupByEmail` depending on how they were granted:
@@ -322,6 +372,17 @@ rather than hardcoding secret references:
    `RW.Core.Import Secret` / `RW.Core.Import User Variable` and use the
    `gcp-auth.yaml` include in templates.
 
+10. **Omitting `CLOUDSDK_BILLING_QUOTA_PROJECT`** -- with a cross-project
+    SA / Workload Identity, gcloud bills API calls to the credential's
+    project and enablement checks fail `SERVICE_DISABLED` against the
+    wrong project. Pin it to `${GCP_PROJECT_ID}` in the suite env (no-op
+    for in-project SAs). Found in `gcp-cloudspanner-instance-health`.
+
+11. **Swallowing a nonexistent command / API error into `[]`** --
+    `bad-cmd 2>/dev/null || echo "[]"` reports "no data" as healthy.
+    Only degrade real permission gaps; for calls whose existence or
+    success isn't guaranteed, validate the response shape and fail loud.
+
 ---
 
 ## Reference Implementation

skills/skill-template-authoring.md

@@ -254,6 +254,12 @@ ro runbook.robot
 
 ## Regeneration
 
+**Running the generator is a required PR step, not optional.** Every
+CodeBundle PR must ship a `SKILL-TEMPLATE.md`, and it must be
+regenerated whenever `runbook.robot`, `sli.robot`, or the scripts change
+-- otherwise the manifest drifts from the source of truth (or is missing
+entirely, as `gcp-cloudspanner-instance-health` shipped without one).
+
 Use the bundled generator after editing robot files:
 
 ```bash
@@ -263,10 +269,25 @@ python3 scripts/generate_skill_md.py /path/to/codecollection --bundle azure-aks-
 
 Writes `SKILL-TEMPLATE.md` and removes legacy `SKILL.md` if present.
 
+**Review the generator output before committing.** It is known to need
+hand-fixes for:
+
+- **Description truncation** -- long `[Documentation]` / frontmatter
+  `description` values get cut off; restore the full sentence.
+- **Sourced helper scripts** -- a task that `source`s a shared helper
+  script may not have that helper attributed under `## Source files`;
+  add it by hand.
+
+Regenerate, then diff and fix these before the manifest goes in the PR.
+
 ---
 
 ## Validation Checklist
 
+- [ ] `SKILL-TEMPLATE.md` exists and was **regenerated** with
+      `generate_skill_md.py` after the latest robot/script edits
+- [ ] Generator output reviewed for description truncation and missing
+      sourced-helper attributions
 - [ ] File is at `codebundles/<name>/SKILL-TEMPLATE.md` (not `SKILL.md`)
 - [ ] Frontmatter includes `kind: skill-template`
 - [ ] `name` matches directory name
@@ -298,3 +319,7 @@ Writes `SKILL-TEMPLATE.md` and removes legacy `SKILL.md` if present.
 
 7. **Using `runner: ro` in frontmatter.** `ro` is devcontainer-only. Production
    uses the platform runner + worker + `runrobot.sh` inside `rw-base-runtime`.
+
+8. **Shipping a PR with no `SKILL-TEMPLATE.md` (or a stale one).** Running
+   `generate_skill_md.py` is a required PR step; regenerate after every
+   robot/script change and hand-fix truncation before committing.

skills/sli-authoring.md

@@ -48,6 +48,50 @@ run it periodically.
 
 ---
 
+## Reading Metrics Safely (learned from live failures)
+
+A structurally-perfect SLI/runbook can still score healthy while reading
+**nothing** if the underlying data call silently fails. These pitfalls
+shipped past review and only surfaced on a live run -- guard against them
+in every check that reads a metric or API. (The
+`CLOUDSDK_BILLING_QUOTA_PROJECT` cross-project trap lives in `auth-gcp.md`;
+the principles here are cloud-agnostic.)
+
+1. **Don't swallow "command not found" / API errors into empty data.**
+   `some-cmd 2>/dev/null || echo "[]"` is only safe when the command
+   *exists* and the sole expected failure is a permission gap. Wrapped
+   around a nonexistent subcommand, a bad flag, or a failed API call it
+   turns a hard error into "no data available" -- which the SLI then
+   scores as **healthy**. Validate the response shape and **fail loud**:
+
+   ```bash
+   if ! echo "$resp" | jq -e '<expected shape>' >/dev/null 2>&1; then
+       echo "ERROR: read failed: $resp" >&2
+       exit 1
+   fi
+   ```
+
+2. **Prefer a first-class, tested call over a guessed one.** If a
+   read-only call has no obvious CLI form, use a shared, tested helper
+   (e.g. a REST call through the already-authenticated session) rather
+   than inventing a subcommand or flag. A command that doesn't exist
+   fails the same way a permission error does -- and pitfall #1 then
+   hides it.
+
+## Report Enrichment, Not Rollup Tasks
+
+Do **not** add a separate "health summary" / rollup task that re-queries
+the same data the individual checks already collected. The SLI already
+computes its aggregate natively (mean of sub-scores in Robot); a rollup
+task just duplicates state/CPU/storage calls, doubles API cost, and drifts
+out of sync with the real checks. Instead, have **each** check enrich the
+report with its own context via `RW.Core.Add to Report` -- full resource
+config JSON, the active thresholds it applied, the inventory it scanned.
+The reader gets richer, per-dimension detail and there's a single source
+of truth per signal.
+
+---
+
 ## `RW_LOOKBACK_WINDOW` on SLIs (platform variable)
 
 `RW_LOOKBACK_WINDOW` controls how far back time-windowed SLI signals look
@@ -458,3 +502,9 @@ Use this checklist when building or reviewing a CodeBundle:
    `Set Variable` argument and can turn dicts into lists; use `Evaluate ... json`
 10. **Confusing Normalize `2` with “×2 interval”** -- second arg is output format;
     platform already supplies seconds sized to the scrape interval
+11. **Silently swallowing a failed data read** -- `cmd 2>/dev/null || echo "[]"`
+    around a nonexistent command or failed API call scores the SLI healthy on
+    zero data; validate response shape and fail loud instead
+12. **Redundant rollup/summary task** -- re-queries data the individual checks
+    already produce and the SLI already averages; enrich each check's report
+    via `Add to Report` instead

skills/test-infra-gcp.md

@@ -326,6 +326,55 @@ labels = {
 }
 ```
 
+### Loading test data after provisioning (metric/utilization checks)
+
+Some checks -- storage utilization, row/object counts, throughput -- only
+have something to read once the resource holds **data**. Provisioning an
+empty instance leaves those metrics at zero and the check untested. Load a
+small amount of data after `apply` with a `null_resource` +
+`local-exec`, and exercise the check via a **threshold override** (e.g.
+`STORAGE_UTILIZATION_THRESHOLD=0`) rather than provisioning realistically
+huge infra:
+
+```hcl
+resource "null_resource" "load_test_data" {
+  depends_on = [google_spanner_database.overloaded_database]
+
+  triggers = {
+    database = google_spanner_database.overloaded_database.name
+  }
+
+  provisioner "local-exec" {
+    command     = "${path.module}/load_test_data.sh"
+    interpreter = ["/bin/bash", "-c"]
+    environment = {
+      TF_VAR_project_id = var.project_id
+      INSTANCE          = google_spanner_instance.overloaded_instance.name
+      DATABASE          = google_spanner_database.overloaded_database.name
+    }
+  }
+}
+```
+
+Keep the loader script robust to CLI arg limits:
+
+- **Hex-encode payloads** and pass them as bytes/`BYTES` columns (or
+  decode inside the query). This avoids gcloud's `--data`/DML string
+  parser choking on quotes/newlines and keeps you under the **~128 KB**
+  total command-line argument limit -- batch inserts rather than one
+  giant statement.
+- Prefer many small `gcloud spanner databases execute-sql` batches (or
+  `bq load` from a temp file) over a single multi-megabyte argument.
+
+**⚠️ Metric lag / flooring caveat.** Provider metrics are not instant and
+often **floor small or young data to 0**. Cloud Spanner's
+`storage/used_bytes`, for instance, can take **hours** to report a nonzero
+value for a freshly-loaded few MB. So a data-loader makes the check
+*exercisable* (via threshold override) but does **not** guarantee a
+nonzero live reading in a short test window. Document the expected lag in
+the bundle's test notes and don't treat a still-zero metric immediately
+after load as a bug.
+
 ---
 
 ## Common Mistakes
@@ -356,6 +405,17 @@ labels = {
    setting `GOOGLE_APPLICATION_CREDENTIALS` short-circuits the
    metadata-server path.
 
+7. **Provisioning an empty resource for a data-dependent check** --
+   storage/count/throughput checks read zero against an empty instance
+   and go untested. Load a little data via `null_resource` + `local-exec`
+   (hex-encoded, batched) and exercise the check with a threshold
+   override -- but account for provider metric lag/flooring (Spanner
+   `used_bytes` can take hours to report nonzero).
+
+8. **Passing large payloads as a single CLI arg** -- gcloud DML/`--data`
+   parsers choke on quotes/newlines and there's a ~128 KB arg limit.
+   Hex-encode and batch, or `bq load` from a temp file.
+
 ---
 
 ## Reference Implementation