From c5c7f0bef88382f1acfdc6ffcf9666bfa9095f2f Mon Sep 17 00:00:00 2001 From: Joana Carvalho Date: Sat, 3 Oct 2026 14:19:24 +0000 Subject: [PATCH] fix(commands): answer an empty scan the same way everywhere, and build nothing (#164) Only /otel-init handled a repo whose scan returns no services. The others failed differently: - /otel-evaluate: "every service reports no OTel" is vacuously true over an empty list, so it said "No existing OTel instrumentation detected. Use /otel-instrument", pointing at a command that cannot help. - /otel-instrument: the "No candidates" branch printed its header and an empty list, no reason. - /otel-collector: generated a repo-root config for a repo with nothing to collect from. - /otel-backend: terraform-gen kept the template's default, or invented an identity (unknown-service, 0.0.0), and built selectors, folder and rule-group names from it. The alerts use no_data_state = "OK", so a selector that matches nothing reports healthy. With --yes the command got past the unconfirmed-attributes prompt, which is how an orchestrated caller reaches it. - /otel-evaluate also wrote .claude/otel-context.json, but the .gitignore update lives in /otel-init Step 4, which an empty scan never reaches, so the cache was left untracked. /otel-init Step 2 now defines the rule once ("Empty scan"): print its message, write nothing (not the cache either), exit 0. It also covers a cache that already holds services: [] and overrides the other commands' "write what it returns" step. evaluate, instrument, collector and backend reference it right after loading context (evaluate before its vacuous check, backend before the --yes-skippable prompt). terraform-gen refuses to emit a module with no real service identity, and forbids both the placeholder and an invented one, as a backstop for a stale cache or a direct call. Adds fixtures/infra-only-repo (the issue's repro shape) and tests/hooks/zero-services-guard.test.sh, wired into lint-hooks; 13 of its 16 checks fail against the previous text. Bumps to 0.8.18. Not verified end to end: whether the scanner actually returns services: [] for that fixture is LLM-judged and not exercised here; the fixture pins the shape and the test pins the commands' response to an empty result. --- .claude-plugin/plugin.json | 2 +- .github/workflows/ci.yml | 10 ++++ ROADMAP.md | 2 +- agents/terraform-gen.md | 12 +++++ commands/otel-backend.md | 6 +++ commands/otel-collector.md | 4 ++ commands/otel-evaluate.md | 5 ++ commands/otel-init.md | 14 ++++- commands/otel-instrument.md | 5 ++ fixtures/infra-only-repo/README.md | 9 ++++ fixtures/infra-only-repo/infra/main.tf | 12 +++++ .../scripts/tool/requirements.txt | 1 + fixtures/infra-only-repo/scripts/tool/run.py | 7 +++ tests/hooks/zero-services-guard.test.sh | 54 +++++++++++++++++++ 14 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 fixtures/infra-only-repo/README.md create mode 100644 fixtures/infra-only-repo/infra/main.tf create mode 100644 fixtures/infra-only-repo/scripts/tool/requirements.txt create mode 100644 fixtures/infra-only-repo/scripts/tool/run.py create mode 100644 tests/hooks/zero-services-guard.test.sh diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 0038983..ec59381 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "otel-as-code", - "version": "0.8.17", + "version": "0.8.18", "description": "OpenTelemetry instrumentation and observability-as-code for any stack", "author": { "name": "radra.gwent", diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 92d5fb7..b9354ca 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -57,6 +57,8 @@ jobs: run: bash tests/hooks/go-instrument-wiring.test.sh - name: Run PHP refusal test (detected, in scope, refused with a reason) run: bash tests/hooks/php-refusal.test.sh + - name: Run zero-services guard contract (every command answers an empty scan the same way; #164) + run: bash tests/hooks/zero-services-guard.test.sh - name: Run collector exporter-auth contract (--exporter-header; --force keeps exporter headers; #171) run: bash tests/hooks/collector-exporter-auth.test.sh - name: Run force-scope contract (--force overwrites only marker-carrying files; #169) @@ -254,6 +256,14 @@ jobs: ! test -f fixtures/python-setuppy-app/Dockerfile ! grep -rq opentelemetry fixtures/python-setuppy-app + - name: Check infra-only-repo fixture (#164 — a repo with no application service) + run: | + test -f fixtures/infra-only-repo/infra/main.tf + test -f fixtures/infra-only-repo/scripts/tool/run.py + [ "$(wc -l < fixtures/infra-only-repo/scripts/tool/requirements.txt)" -eq 1 ] + ! grep -qiE "flask|fastapi|django|uvicorn|serve_forever|app.run" fixtures/infra-only-repo/scripts/tool/run.py + ! grep -rqi opentelemetry fixtures/infra-only-repo + - name: Check nextjs-brownfield-api-helper fixture (#169 — bootstrap vs hook vs API call site) run: | grep -q "new NodeSDK" fixtures/nextjs-brownfield-api-helper/src/telemetry.node.ts diff --git a/ROADMAP.md b/ROADMAP.md index 072a847..1fe95c1 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -8,7 +8,7 @@ > of the same shape, and every concrete line item that isn't done yet has a linked tracking issue — > comment there rather than only in this file if you want to weigh in. -Current release: **0.8.17**. Node.js, Python, Java, .NET, Go, Ruby, and Rust (axum) instrumentation, Terraform +Current release: **0.8.18**. Node.js, Python, Java, .NET, Go, Ruby, and Rust (axum) instrumentation, Terraform for Grafana, Datadog, New Relic, and Dash0, semconv pinned at 1.44.0. Since 0.2.0 the command set has grown past generation: `/otel-uninstrument` rolls instrumentation back behind an ownership marker, `/otel-remediate` proposes a deployment-config diff for services that have no application diff --git a/agents/terraform-gen.md b/agents/terraform-gen.md index 45e48a9..4adf5cb 100644 --- a/agents/terraform-gen.md +++ b/agents/terraform-gen.md @@ -26,6 +26,18 @@ to the target directory. You run `terraform fmt` and `terraform validate` after line shown under "Required header in every main.tf" so anyone reading the module later can see the queries and SLO targets were not built from confirmed attributes. +## Refuse a module with no real service + +A module is built from a service's identity (`name`, `version`, `namespace`): PromQL selectors, +the dashboard uid, folder and rule-group names, and alert labels all come from it. If +`context.services` is empty, or the service has no `name`, **write nothing and return the reason** +("no service identity in the context: run /otel-init on an application repository"). Never keep a +template placeholder such as `` or `` as a variable default, and +never invent an identity (`unknown-service`, `0.0.0`, one inferred from unrelated files). The +result looks valid, but every selector matches nothing, so the dashboard is empty and an alert +with `no_data_state = "OK"` reports a healthy service that does not exist (#164). The dispatching +command applies the same rule first; this is the backstop for a stale cache or a direct call. + ## kind parameter behavior When `kind` is not `all`, generate only the specified resource type: diff --git a/commands/otel-backend.md b/commands/otel-backend.md index a932f62..b4c6741 100644 --- a/commands/otel-backend.md +++ b/commands/otel-backend.md @@ -51,6 +51,12 @@ Check `.claude/otel-context.json`. Apply the freshness rule from `/otel-init` St what it returns — a refresh is a merge, never a replace (see the cache ownership contract in `agents/repo-context-scanner.md`). +If the scan or the cache has **no services**, apply `/otel-init` Step 2's "Empty scan" rule +before anything else in this step, including the confirmation prompt below: print its message, +write nothing, exit 0. A module needs a real service identity to build selectors, folder and +alert names from; with none, `terraform-gen` would keep a template placeholder or invent one, +and the resulting dashboard and alerts match nothing (#164). + If `context.confirmedAt` is null (business attrs not confirmed): - Print: "⚠ Business attributes have not been confirmed yet. Running /otel-business-attrs first will improve the generated dashboard queries and SLO targets." diff --git a/commands/otel-collector.md b/commands/otel-collector.md index c31ad87..af63085 100644 --- a/commands/otel-collector.md +++ b/commands/otel-collector.md @@ -58,6 +58,10 @@ Check `.claude/otel-context.json`. Apply the freshness rule from `/otel-init` St what it returns — a refresh is a merge, never a replace (see the cache ownership contract in `agents/repo-context-scanner.md`). +If the scan or the cache has **no services**, apply `/otel-init` Step 2's "Empty scan" rule: +print its message, write nothing, exit 0. A Collector config describes what receives telemetry +from services; for a repo with none it would be a repo-root file that collects from nothing. + ## Step 3: Determine output path Write to: `otelcol-agent.yaml` (mode=agent) or `otelcol-gateway.yaml` (mode=gateway) diff --git a/commands/otel-evaluate.md b/commands/otel-evaluate.md index 711f1ce..59447c8 100644 --- a/commands/otel-evaluate.md +++ b/commands/otel-evaluate.md @@ -15,6 +15,11 @@ Check `.claude/otel-context.json`. Apply the freshness rule from `/otel-init` St what it returns — a refresh is a merge, never a replace (see the cache ownership contract in `agents/repo-context-scanner.md`). +If the scan or the cache has **no services**, apply `/otel-init` Step 2's "Empty scan" rule: +print its message, write nothing, exit 0. Do this before the check below: "every service reports +no OTel" is vacuously true over an empty list, and it would send the user to `/otel-instrument`, +which cannot do anything here either. + Then decide whether there is anything to audit. If every service reports `existingOtel.hasTraces = false` AND `hasMetrics = false` AND `hasLogs = false`, **verify that on disk before believing it.** These flags are only as fresh as the last scan, and a cache that diff --git a/commands/otel-init.md b/commands/otel-init.md index e7db66e..8c501d7 100644 --- a/commands/otel-init.md +++ b/commands/otel-init.md @@ -88,10 +88,20 @@ Wait for the JSON response. Never write a scanner response whose user-owned fiel over a cache where they were not — if that happens the merge did not run; re-dispatch with `priorContext` rather than writing the result. -If the subagent returns an empty `services` array: +**Empty scan (the shared rule).** If the subagent returns an empty `services` array — or a +cache you loaded already has `services: []`: - Print: "No services detected. Is this an application repository? Check the service detection rules in the otel-as-code docs." -- Exit. +- **Write nothing and exit 0.** In particular do NOT write the scan to `.claude/otel-context.json`: + the `.gitignore` update below lives in Step 4, which an empty scan never reaches, so a cache + written here is left as an untracked file in the user's repo. +- `/otel-evaluate`, `/otel-instrument`, `/otel-collector` and `/otel-backend` apply this same rule + right after they load or refresh the context (#164), and it overrides their "write what it + returns" instruction for a scan with no services. They reference it rather than restate it, + because a repo with no application service has nothing to audit, instrument, collect from, or + build a dashboard for, and each of them used to fail differently: a vacuous "no instrumentation + detected" that points back at a command that cannot help, an empty candidate list with no + reason, and a module built for a placeholder service. ## Step 3: Present detected service boundaries diff --git a/commands/otel-instrument.md b/commands/otel-instrument.md index adb95d3..fa7d817 100644 --- a/commands/otel-instrument.md +++ b/commands/otel-instrument.md @@ -49,6 +49,11 @@ Check `.claude/otel-context.json`. Apply the freshness rule from `/otel-init` St `Skill` tool and cannot read the guidance itself. - Do NOT print the /otel-init success message; just run the scan silently. +If the scan or the cache has **no services**, apply `/otel-init` Step 2's "Empty scan" rule: +print its message, write nothing (not the cache either), exit 0. Step 2's "No candidates" branch +is for a repo that HAS services none of which can be instrumented, where each one carries a +reason; it must not be reached with an empty list. + ## Step 2: Select the target SERVICE Pick a service first; its language follows from it. Resolving a language alone settles nothing diff --git a/fixtures/infra-only-repo/README.md b/fixtures/infra-only-repo/README.md new file mode 100644 index 0000000..72d4e5a --- /dev/null +++ b/fixtures/infra-only-repo/README.md @@ -0,0 +1,9 @@ +# infra-only-repo + +The shape from #164: an infrastructure repository with no application service. One OpenTofu +stack, docs, and a directory of one-shot Python scripts whose only manifest is a one-line +`requirements.txt`. No web framework, no entry point a process manager would start, no Dockerfile. + +The scanner is expected to return `services: []` for it, and every command that needs a service +must then say what `/otel-init` says and write nothing, instead of dead-ending, printing an empty +candidate list, or building a module for a placeholder service. diff --git a/fixtures/infra-only-repo/infra/main.tf b/fixtures/infra-only-repo/infra/main.tf new file mode 100644 index 0000000..ee0e41b --- /dev/null +++ b/fixtures/infra-only-repo/infra/main.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + random = { + source = "hashicorp/random" + version = "~> 3.6" + } + } +} + +resource "random_id" "suffix" { + byte_length = 4 +} diff --git a/fixtures/infra-only-repo/scripts/tool/requirements.txt b/fixtures/infra-only-repo/scripts/tool/requirements.txt new file mode 100644 index 0000000..8549900 --- /dev/null +++ b/fixtures/infra-only-repo/scripts/tool/requirements.txt @@ -0,0 +1 @@ +markdown>=3.5 diff --git a/fixtures/infra-only-repo/scripts/tool/run.py b/fixtures/infra-only-repo/scripts/tool/run.py new file mode 100644 index 0000000..21d2594 --- /dev/null +++ b/fixtures/infra-only-repo/scripts/tool/run.py @@ -0,0 +1,7 @@ +"""One-shot maintenance script: render a markdown file to HTML. No server, no entry point.""" +import sys + +import markdown + +if __name__ == "__main__": + print(markdown.markdown(open(sys.argv[1]).read())) diff --git a/tests/hooks/zero-services-guard.test.sh b/tests/hooks/zero-services-guard.test.sh new file mode 100644 index 0000000..51b9d7d --- /dev/null +++ b/tests/hooks/zero-services-guard.test.sh @@ -0,0 +1,54 @@ +#!/usr/bin/env bash +# tests/hooks/zero-services-guard.test.sh +# Locks the #164 contract: a repo whose scan returns no services is answered the same way by every +# command that needs a service, and nothing is written. Before this, /otel-evaluate printed a +# vacuous "no instrumentation detected", /otel-instrument printed an empty candidate list, +# /otel-collector wrote a config for nothing, and /otel-backend built a module around a +# placeholder () or invented (unknown-service) identity whose alerts sat in +# no_data_state = "OK". +set -uo pipefail +cd "$(dirname "$0")/../.." +pass=0; fail=0 +check() { if eval "$2"; then echo "PASS: $1"; pass=$((pass+1)); else echo "FAIL: $1"; fail=$((fail+1)); fi; } + +INIT="commands/otel-init.md" +TFGEN="agents/terraform-gen.md" +FIX="fixtures/infra-only-repo" + +check "otel-init defines the shared Empty scan rule: message, write nothing, exit 0" \ + 'grep -qF "**Empty scan (the shared rule).**" "$INIT" && grep -qF "**Write nothing and exit 0.**" "$INIT"' +check "the rule covers a cache that already has services: []" \ + 'grep -qF "cache you loaded already has \`services: []\`" "$INIT"' +check "the rule forbids writing the scan to the cache (the .gitignore update is not reached)" \ + 'grep -qF "do NOT write the scan to \`.claude/otel-context.json\`" "$INIT"' +check "the rule overrides the commands' 'write what it returns' instruction" \ + 'grep -qF "it overrides their \"write what it" "$INIT"' + +for c in otel-evaluate otel-instrument otel-collector otel-backend; do + check "$c applies the Empty scan rule by reference, not by restating it" \ + 'grep -qF "apply \`/otel-init\` Step 2'"'"'s \"Empty scan\" rule" "commands/'"$c"'.md"' +done + +check "evaluate applies it BEFORE the vacuous 'every service reports no OTel' check" \ + '[ "$(grep -n "Empty scan" commands/otel-evaluate.md | head -1 | cut -d: -f1)" -lt "$(grep -n "Then decide whether there is anything to audit" commands/otel-evaluate.md | cut -d: -f1)" ]' +check "backend applies it BEFORE the unconfirmed-attributes prompt (so --yes cannot get past it)" \ + '[ "$(grep -n "Empty scan" commands/otel-backend.md | head -1 | cut -d: -f1)" -lt "$(grep -n "If \`context.confirmedAt\` is null" commands/otel-backend.md | cut -d: -f1)" ]' +check "instrument says the No candidates branch is not for an empty list" \ + 'grep -qF "must not be reached with an empty list" commands/otel-instrument.md' + +check "terraform-gen refuses a module with no real service" \ + 'grep -qF "## Refuse a module with no real service" "$TFGEN"' +check "terraform-gen forbids both the template placeholder and an invented identity" \ + 'grep -qF "Never keep a" "$TFGEN" && grep -qF "unknown-service" "$TFGEN" && grep -qF "" "$TFGEN"' + +# --- fixture: the issue's repro shape, structurally --- +check "fixture has no application manifest beyond a one-line requirements.txt" \ + '[ "$(find "$FIX" -type f \( -name package.json -o -name pyproject.toml -o -name go.mod -o -name pom.xml -o -name Gemfile -o -name Cargo.toml -o -name "*.csproj" -o -name Dockerfile \) | wc -l)" -eq 0 ] && [ "$(wc -l < "$FIX/scripts/tool/requirements.txt")" -eq 1 ]' +check "fixture's script has no web framework and no server" \ + '! grep -qiE "flask|fastapi|django|uvicorn|http.server|serve_forever|app.run" "$FIX/scripts/tool/run.py"' +check "fixture carries an infrastructure stack and no OTel" \ + 'test -f "$FIX/infra/main.tf" && ! grep -rqi opentelemetry "$FIX"' + +echo "" +echo "Results: $pass passed, $fail failed" +[ "$fail" -eq 0 ]