feat(metrics): install-inventory gauges and OTLP export verification

[chenmingwei23]

Problem / Motivation

Nothing samples what an install has actually configured. process_gauges answers
"how is this process behaving" -- threads, file descriptors, RSS, GC -- but an
operator running Kiro Crew on more than one machine has no way to see that a host
stopped scheduling crons, that skills and knowledge documents are piling up on one
box and absent on another, or that a feature switch differs between hosts. The
dashboard shows one machine's counts on request; nothing samples them over time, so
config drift between hosts is only visible by opening each dashboard in turn.

Scope, stated up front because the metric names invite the opposite reading:
this is operator-fleet telemetry, not project-wide adoption analytics. Collection is
off by default, egress is a second opt-in, and OTLP needs the kirocrew[otlp]
extra, so any aggregate over these gauges describes one operator's own opted-in
machines. Install analytics structurally cannot ride this trunk -- beacon.py is
that channel, and its docstring plus the spec's "Why it is NOT part of the OTEL
trunk" section give four independently disqualifying reasons. That boundary is now
written into the module docstring, the spec, and the operator guide so it is not
re-litigated later.

What this answers today, and what it does not. Hosts are separable at a moment:
service.instance.id comes from the SDK's default resource, and the version pinned
for the kirocrew[otlp] extra (1.44.0) supplies it. What is NOT available yet is the
longitudinal per-host view, because that id is regenerated per process, so a host's
series restarts with its gateway. The fix is a persisted install-scoped resource
identity and it is already in flight as #7266, which makes service.instance.id the
persisted beacon.install_id. Nothing here needs to change when it lands: the
resource assertions in this PR compare against the provider's own resource rather
than a fixed list, so new attributes are covered the moment they appear. That work
is also the precondition for ever removing the reporter election below, though not a
drop-in replacement for it: a stable install id makes downstream dedup possible
(max by (install)), but that is query-time discipline -- the data would still carry
one copy per process, and a naively written fleet sum would read N times the truth,
where the election makes it correct at the point of emit. Until then
these gauges are honest as fleet aggregates and as point-in-time per-instance
readings, and the operator guide says so where an operator will hit it.

There is a second, narrower gap. test/metrics/test_local_exporter.py already proves
serialization, but it proves it of the exporter alone: built directly and fed from a
provider assembled in the test. Nothing asked the same question of the live build
-- that the roster provider._build_recorder() registers actually arrives, carrying
the attribute values the modules declare, at the temporality each instrument kind
requires. An instrument can be missing outright, and an attribute value or a
temporality setting can be correct in the SDK's in-memory view and still be dropped
or wrong by the time it leaves the process, and nothing would catch it.

Why it matters

An operator cannot currently distinguish a host that is idle from one whose cron
store stopped loading, or notice that one machine's skills tree drifted, without
checking each dashboard by hand. These are exactly the questions a time series
answers cheaply and a point-in-time panel cannot.

Without a serialization test, the failure mode is silent and lands on operators:
metrics appear to work locally, then arrive at a backend missing a label or with
counters that look like they reset constantly. That is the class of defect that
survives to production because the in-memory assertion passed.

What changed (motivation -> approach -> change)

Inventory gauges. src/kiro_crew/metrics/inventory_gauges.py adds eight
observable gauges under kirocrew.inventory.*, mirroring process_gauges
deliberately rather than inventing a second pattern: callbacks run only when a
reader collects, registration happens on _build_recorder's live path so the
telemetry.enabled consent gate covers them, raw readers stay SDK-free so they are
unit-testable without a pipeline, and a probe that cannot answer yields no
observation rather than a fake zero.

Cost drove most of the design. A callback runs once per export interval per
reader, so an install with an OTLP destination configured enters these on two
ticker threads concurrently. Every probe is O(1) in-memory, one small file parse, or
explicitly TTL-cached, and the module documents the cost class of each:

• crons: reads crons.json through cron's own record helpers -- the same ones
  count_enabled_from_disk uses, so it cannot drift from what the scheduler
  considers enabled. Deliberately not list_jobs, which re-arms the asyncio timer
  and from a reader thread would raise after cancelling the live timer, stopping
  every scheduled job.
• skills and knowledge are the two genuinely expensive probes (a recursive walk, a
  SQLite open) and are cached 300s. The knowledge probe opens SQLite read-only
  and only when the database already exists: constructing a KnowledgeStore runs
  schema init plus graph load and would create the database on an install that
  never ingested anything.
• monitor loops read the auto-nudge registry via a materialized snapshot, which is
  safe from a foreign thread even though the registry's asyncio.Lock is not.

Privacy shaped one choice, and rejected a second. MCP server names are read to
classify and then discarded -- only first_party / third_party counts leave,
because a roster is user-chosen text and would be both unbounded cardinality and a
disclosure of what the user has installed. Knowledge and lesson counts, by contrast,
publish the raw number: banding them was the tempting move on the theory that an
exact count is a slowly-changing fingerprint, but that argument does not survive the
scope above. These metrics only reach the collector the operator configured, about
machines that operator already owns and can identify by far more than a document
count -- so the band protects nobody, while it costs a series per band, renders
growth inside a band (the drift the gauge exists for) as a flat line, and fixes the
boundaries at emit time. A backend can band a raw count at query time; it cannot
recover a count from a band. The product's own thresholds (50 = knowledge.max_sources
default, 200 = lesson prune ceiling) are named in the operator guide instead.

One publisher per install. Install-level facts are identical across the several
processes an install runs at once -- gateway, MCP gateway daemon, spawned agents -- so
publishing them from each would count one install once per process: a fleet sum reads
N times the truth, and a fleet average is dragged toward whichever installs happen to
run the most processes.
Deduplicating downstream needs an install-scoped resource identity that does not exist
yet, so exactly one process publishes: the gateway calls mark_install_reporter(), and
every callback checks the claim at COLLECT time rather than at registration, so a
recorder built before the claim publishes nothing until it lands instead of publishing
wrongly.

Its placement is part of that contract, not an incidental detail. Nothing enforces the
election at runtime, and an unclaimed install is silent in both directions -- the
callbacks return before probing, so even probe.failures stays empty and the family
reads exactly like a host that stopped exporting. So the claim sits in
GatewayOrchestrator.run() at a point no branch guards, and
test_reporter_claim_is_unconditional parses the gateway to assert that: called
exactly once, from run(), without passing through a conditional (try is allowed --
telemetry must never block boot).

Three points where the intended design did not survive contact with the code, all
resolved toward reporting something true rather than something convenient:

1. There is no memory.enabled flag. The embedding provider is coerced to a
   real value on load, so memory is structurally always on and an "enabled" gauge
   could only ever read 1. The gauge reports memory.migrated instead, which is a
   real question (how far the vector-store migration has reached).
2. telemetry.enabled is excluded from the toggle set: this module only runs
   inside the consent gate, so it is a tautology dressed as a measurement.
3. The first-party MCP set reuses mcp_discovery._MANAGED_SERVER_NAMES rather than
   restating a name list that would drift silently the first time a managed server
   is renamed.

Verification. test/metrics/test_otlp_wire_e2e.py closes the serialization gap
in two tiers. Tier 1 drives the real _build_recorder live path through its real
exporter and asserts the instrument roster, resource-attribute fidelity, the
closed attribute enums, and temporality. Fidelity is asserted by comparing the
serialized resource against the live provider's own resource rather than a hardcoded
list, so an attribute added to the resource later is covered the moment it lands and
one the exporter silently drops fails here. Tier 2 posts through a real
OTLPMetricExporter to an in-process receiver bound to loopback and decodes the
protobuf; it skips unless the optional kirocrew[otlp] extra is installed.

The two tiers together also pin a boundary neither could pin alone:
kirocrew.process.start_time is a host-local, reboot-unique token the JSONL
exporter stamps per record, so tier 1 asserts it IS on the local shard and tier 2
asserts it is absent from the OTLP payload. Either assertion alone passes while the
attribute sits on the wrong side of the boundary.

Those two tiers are the whole automated surface, and they stop at the process
boundary. Driving a real collector needs a downloaded binary and outbound network, so
it cannot be a test in a network-isolated CI, and this PR does not ship a script for
it either: the operator guide describes how to do that check by hand against whatever
collector you already run, which is the form that survives a version bump. See the
Tests section for the one-time run that was done during development, and what it is
and is not evidence of.

Production wiring outside the new module is three hunks. provider.py registers the
inventory gauges next to the process ones, in its own try block so a failure in
either family cannot cost the other -- they read entirely different subsystems.
slack/gateway.py claims the reporter role, as above. And cron.py gains
enabled_count_from_disk(path) -> (count, loadable), a module-level sibling of
unhealthy_jobs_from_disk that becomes the single owner of the enabled-count
reduction: CronService.count_enabled_from_disk now calls it and keeps the count,
this probe calls it and needs loadable too. Both sides previously carried their own
spelling of that loop.

Docs: docs/guides/telemetry-otlp-export.md covers pointing the exporter at a
collector and on to CloudWatch, Datadog, or any OTLP-compatible backend, with
collector config samples (explicitly marked illustrative and not tracked, since
vendor options drift with releases this repo does not follow), the two separate
consent switches, what is and is not exported, and
OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE -- the setting that decides
whether a backend accepts the data, which works precisely because the reader builder
passes no explicit temporality. docs/system-specs/modules/metrics.md gains a row
per new instrument, so the spec moves with the code.

Tests

test/metrics/test_inventory_gauges.py (43 cases), four layers matching the
module:

• Raw readers: each probe's happy path, and its None-vs-zero split individually
  pinned. The split is not just none-vs-zero but SILENT-vs-COUNTED: an absent source
  is a quiet gap because nothing is broken (no auto-nudge service in this process, a
  knowledge database on an install that never ingested, a missing cron store on a
  fresh install), while a source that is PRESENT and unreadable is a fault and
  increments probe.failures before returning None. An unparseable crons.json is
  the case in point -- cron's own _read_job_records calls it a fault, and a silent
  gap there would be indistinguishable from a host that stopped exporting, which is
  the confusion the counter exists to resolve. Pinned at the reader level and again
  through a real collection.
• Cost contract: the TTL cache produces once within its window, re-produces
  after expiry, and caches a None answer so an install that can never answer does
  not re-pay an expensive probe every cycle.
• Side-effect ratchet: the knowledge probe must not create the database, and its
  read-only connection must refuse a write.
• Anti-typo ratchet: test_every_declared_toggle_resolves_against_a_real_config
  asserts every declared toggle resolves against a real config. The runtime omits an
  unresolvable key (a renamed field must read as a missing series, never as a switch
  someone turned off), which is right but would hide a typo forever; this is what
  makes the omission observable.
• Drift ratchet: asserts no managed server name is restated in this module, so
  the constant stays the single owner.
• Privacy ratchet: an injected distinctive server name must appear in no
  attribute key or value.
• Series-shape ratchet: the two store-backed counts must arrive as one
  attribute-free series carrying the pinned reading, at the reader level and again on
  the wire -- a reintroduced band fails both halves (constant 1, plus an attribute).
• Call-site ratchet: test_reporter_claim_is_unconditional parses the gateway and
  asserts mark_install_reporter() is called exactly once, from run(), without
  passing through a conditional -- the one invariant that has no runtime enforcement
  and no failure signal when it breaks.
• Failure isolation: a raising or unavailable reader yields a gap while every
  other metric survives; registration never raises even on a hostile meter; and an
  inventory registration failure does not cost the process gauges.

test/metrics/test_otlp_wire_e2e.py (14 cases) as described above, including the
privacy ratchet repeated at the serialization boundary -- the reader-level test
proves names are discarded, this proves nothing downstream puts them back.

Both e2e tiers pin their probe readings rather than reading host state, so the
assertions are about the serialization contract and cannot flake on whether the
machine running them happens to have a knowledge database or any crons.

Manual verification

• pytest test/metrics/ -q run three times: 549 passed each time.
• flake8, isort, mypy clean on the changed files; the repo's own
  scripts/check_black_formatting.py passes with the changed files in scope, and
  scripts/docs-lint.sh passes.
• Per-file coverage on the new module: 98% (195 statements, 2 missed), against the
  80% floor.
• OTLP tier 2 was run with the optional extra installed locally (it skips without
  it), so the skipped-in-CI tier is not untested code.
• A real otelcol was driven end to end ONCE during development -- checksum verified,
  collector started, one export pushed through it, 16 instruments confirmed received.
  That is a one-time result, not a gate: nothing in this PR re-runs it, and it does not
  protect against a future regression. It is reported here because it is the only
  evidence that a real collector accepts this payload; the repeatable half of that
  check is tier 2, which decodes the real exporter's protobuf in CI.
• Semgrep suppressions were verified locally in both directions (each rule fires
  under --disable-nosem and is silent without it), rather than by re-running CI.

Related Issues

Refs #7232 #7257

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of e18f40bf539693f7bbff490bd4a20fd0f0d2d965 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real fleet-observability gap, closed with the existing process-gauges pattern, honest failure semantics, and every known limitation named rather than hidden.

Suggestions

• The cron probe got the right treatment — a promoted public enabled_count_from_disk owned by cron.py — but the other probes reach into private state (service._loops, loader._iter_visible(), mcp_discovery._MANAGED_SERVER_NAMES, the hardcoded sources table). Each has a ratchet test, but the owning modules still carry an invisible dependent; promoting the same kind of small public read-only accessor there, as follow-ups, would put the drift contract where the change happens instead of in this module's tests.

[DESIGN-REVIEWED] e18f40b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed e18f40bf539693f7bbff490bd4a20fd0f0d2d965 — this comment is updated in place on each push.

Review details

The repo's own snapshot.py:612-617 comment documents the exact failure mode of raw interpolation (?/# truncate the path), and kiro_cli.py:163 / kiro_prerequisite.py:706 / onboarding_import.py:2874 all use pathname2url/as_uri() to build read-only URIs Windows-safely. The new line at inventory_gauges.py:543 uses the naive form. I've confirmed the call path (read_knowledge_documents → _ttl_cached → _read_knowledge_documents_uncached → sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)), that db_path.exists() passes on Windows, and that the swallowed OperationalError caches None and increments PROBE_KNOWLEDGE.

This is a real correctness defect, but the exception is caught — no crash, data loss, corruption, or security hole — so it does not meet the BLOCKING bar. It is advisory.

One advisory finding: the knowledge-documents probe builds its SQLite URI by raw interpolation, misreporting healthy Windows installs as a broken probe.

FINDING — src/kiro_crew/metrics/inventory_gauges.py:543 — sqlite3.connect(f"file:{db_path}?mode=ro", uri=True, ...) interpolates a native path raw, so on Windows the drive-letter/backslash db_path (and any POSIX home containing ?/#/%) is not a valid URI filename → OperationalError → _ttl_cached caches None and bumps probe.failures{probe="knowledge"}, so a healthy install with an ingested DB reads as a permanently broken probe — the exact drifted-vs-dark confusion the family exists to resolve → Fix: build the URI the way the repo's other read-only openers do, f"file:{urllib.request.pathname2url(str(db_path))}?mode=ro" (cf. kiro_cli.py:163) or f"{db_path.absolute().as_uri()}?mode=ro" (cf. snapshot.py:617).

[OPUS-REVIEWED] e18f40b

Verdict parsed from the review's SHA-scoped output markers for commit e18f40bf539693f7bbff490bd4a20fd0f0d2d965.

False positive or not applicable? A repository writer can comment:
/ai-review override fable e18f40bf539693f7bbff490bd4a20fd0f0d2d965: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of e18f40bf539693f7bbff490bd4a20fd0f0d2d965 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification greps done. The findings survived or died as follows: enabled_count_from_disk has exactly 2 real consumers and removes a duplicated loop; beacon.py exists as the separate analytics channel the description cites; test_local_exporter.py really does construct the exporter directly (11 sites), so the new e2e tier is not a second spelling; _ttl_cached's ttl parameter has 2 real callers, both passing the same constant.

First-Principles-Verdict: PASS

Fleet-drift visibility with every rider declared, each symptom-level stopgap naming its cause (#7266), and one duplicated loop deleted along the way.

What this change ships

Intent: let an operator see, over time, what each of their opted-in machines has configured — an ADDITION, declared as such.

1. Eight kirocrew.inventory.* gauges reach the operator's collector once both opt-ins are set — justified
2. probe.failures counter separates a broken probe from a host gone dark — justified (knowledge/monitor gaps are otherwise ambiguous with "never ingested")
3. Only the gateway publishes install-scoped inventory (reporter election) — justified; stopgap declared, cause (feat(metrics): identity, version, and environment resource attributes #7266 install-scoped resource id) named
4. First failure per probe logs at WARNING, repeats at debug — justified
5. New operator guide for OTLP export and end-to-end verification — declared; vendor YAML self-fenced as untracked
6. Live-build wire-verification tests, two tiers — justified; not a duplicate of test_local_exporter.py, which constructs JsonlMetricExporter directly (11 sites), never the live build
7. Cron enabled-count loop unified into cron.enabled_count_from_disk (2 consumers) — justified deletion of a second spelling
8. Same-commit spec updates (metrics.md, learn-cron-dashboard.md) — mandated by AGENTS.md

The one structural asymmetry I checked and accepted: probe.failures covers only the 8 inventory probes, not the 9 kirocrew.process.* instruments — but those read the process's own /proc/gc, where a gap is a documented platform property rather than a realistic failure, so the confinement is derived, not an unfixed sibling.

Subtractions

• Drop _ttl_cached's ttl parameter — both real callers (inventory_gauges.py:502, :562) pass _EXPENSIVE_TTL_SECS; read the module constant inside and take (key, produce).

[FIRST-PRINCIPLES-REVIEWED] e18f40b

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of e18f40bf539693f7bbff490bd4a20fd0f0d2d965 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e18f40b

False positive or not applicable? A repository writer can comment:
/ai-review override gpt e18f40bf539693f7bbff490bd4a20fd0f0d2d965: <one-sentence reason>

[chenmingwei23]

Round 1 dispositions - d34a9b170

Both reds were real and both were in scripts/telemetry/otlp_collector_e2e.py.

Brand Name Gate - FIXED

Three added lines spelled the product name joined in prose. Corrected to Kiro Crew at
lines 2, 16, and the _export_once docstring. The remaining KiroCrewConfig occurrences
are the class identifier, which the gate already exempts and did not flag.

SAST (Semgrep), 2 findings - FIXED

Rather than suppress on assertion, each suppression is backed by a guard that makes the
claimed property enforced. Both rules already have established precedents in this repo
(docker/seccomp/gen_profile.py:99, scripts/run_scoped_tests.py:427), so the annotation
style follows the existing convention.

1. dynamic-urllib-use-detected (urllib.request.urlopen). The URL cannot be a
literal, because the release asset name is platform-dependent. It can, however, be
constrained. _download now refuses any URL that is not under the pinned _RELEASE_BASE
(a hardcoded https GitHub release tag) before it fetches:

if not url.startswith(f"{_RELEASE_BASE}/"):
    _fail(f"refusing to fetch outside the pinned release: {url}")

That turns "this script only ever downloads from one pinned release" from an incidental
property of today's two call sites into one a future edit cannot widen silently. The
nosemgrep comment names that guard and the subsequent SHA-256 verification.

2. dangerous-subprocess-use-tainted-env-args (subprocess.Popen). Worth noting this
repo has its own rule for exactly this pattern, kirocrew.download-then-subprocess in
semgrep/supply-chain.yaml, and it states the required mitigation: "Ensure the file has
verified integrity (SHA-256 check) before execution." That was already the design:
_fetch_collector verifies the archive against the checksums published with the pinned
release and aborts on any mismatch, so reaching the exec site means the bytes matched.

The gap was that the guarantee lived in a function far above the exec. The exec site now
re-asserts the two structural facts itself:

if binary.parent != workdir or not binary.is_file():
    _fail(f"refusing to execute {binary}: not the verified binary in {workdir}")

argv remains a list, never a shell string, so there is no shell to inject into.

Verification

• pytest test/metrics/ -q: 541 passed.
• flake8 / isort / mypy clean; scripts/check_black_formatting.py passes with 8 files in scope.
• scripts/telemetry/otlp_collector_e2e.py re-run end to end AFTER adding both guards:
  checksum verified, collector started, 16 instruments received, PASS. The guards do not
  break the path they protect.

No /ai-review override used.

[chenmingwei23]

Round 2 dispositions - 9483c7c34

SAST (Semgrep) - FIXED (my round-1 fix was placed wrong)

Round 1 added the right nosemgrep annotations but placed them badly: I put three
explanation lines between the nosemgrep comment and the code. Semgrep only honors the
annotation on the reported line or the line immediately preceding it, so both suppressions
were inert and the finding count did not move.

This repo already documents the sharper half of that trap, in the comment above
scripts/run_scoped_tests.py:427: "across lines moved the report onto the argument line,
where a comment on the preceding line no longer suppressed it." For the Popen finding
semgrep reports the argv line, not the subprocess.Popen( line, so the annotation has
to sit directly above the argv element.

Corrected placement:

• urllib.request.urlopen: explanation lines moved above, nosemgrep now immediately
  precedes the with statement (the same shape as docker/seccomp/gen_profile.py:98-100).
• subprocess.Popen: nosemgrep moved inside the call, immediately above
  [str(binary), "--config", str(config_path)], which is the line semgrep reports.

Verified locally rather than by another CI round. With semgrep 1.175 against each rule,
each one fires with --disable-nosem and is silent without it:

--disable-nosem: dangerous-subprocess-use-tainted-env-args   311| [str(binary), ...]
--disable-nosem: dynamic-urllib-use-detected                 127| with urllib.request.urlopen(...)
default (nosemgrep active): 0 findings

That two-directional check is the point: an empty scan alone would not distinguish
"suppressed" from "rule not in this config".

The guards added in round 1 are unchanged and still carry the actual mitigation: the
pinned-_RELEASE_BASE prefix check before any fetch, and the verified-binary-in-workdir
check before any exec.

Backend Lint & Type Check (3.10) - FIXED

The subprocess-encoding gate flagged subprocess.Popen(..., text=True) with no
encoding=, which decodes with the platform locale code page. Real: collector stdout would
come back mojibake, or raise, on a host whose default codec is not UTF-8. Pinned to
encoding="utf-8", errors="replace" (the gate's sanctioned form) rather than
**UTF8_TEXT, to avoid adding a top-level kiro_crew import to a script that deliberately
defers those into functions so --help works without the package installed.
scripts/check_subprocess_encoding.py now passes with 8 files in scope.

Backend Lint & Type Check (3.12) showed cancelled on the previous SHA; that was a
superseded dispatch, not a finding.

Verification

• pytest test/metrics/ -q: 541 passed.
• flake8 / isort / mypy / black clean; black gate and subprocess-encoding gate both pass.
• scripts/telemetry/otlp_collector_e2e.py re-run end to end after the exec-call change:
  16 instruments received, PASS.

No /ai-review override used.

[chenmingwei23]

Round 3 dispositions - fb825e0ca

First Principles raised CONCERNS (advisory). I checked its premise against the code before
acting, and it is correct: the framing was an over-claim. One concern is fixed, one
rebutted with evidence, one partially accepted.

1. "The gauges can never answer the stated question" - FIXED (premise was wrong, mine)

Verified, and the reviewer is right. beacon.py:21-44 and the spec's own "Why it is NOT
part of the OTEL trunk" section both state four independently disqualifying reasons adoption
data cannot ride the metrics trunk, including that OTLP egress lives in an optional extra so
it "would measure only users who installed an optional extra." My PR body claimed these
gauges make project-wide adoption questions answerable. They cannot, and saying so put this
change in conflict with a boundary the repo had already written down twice.

The gauges themselves are unchanged and still justified - they serve exactly the job the
rest of kirocrew.* serves, an operator observing machines they run, where a host that
stopped scheduling crons or whose skills tree drifted is genuinely invisible today. What
changed is that the claim now matches that job, in all four places a future reader would
look:

• inventory_gauges.py module docstring: reframed to operator-fleet, plus an explicit
  "WHAT THESE CANNOT ANSWER" paragraph naming beacon.py as the install-analytics channel
  and summarising its four reasons.
• docs/system-specs/modules/metrics.md: a scope paragraph on the family, ending
  "Do not later 'promote' these to answer install-population questions."
• docs/guides/telemetry-otlp-export.md: a "What these can and cannot tell you" section.
• The PR description, which carried the original over-claim.

I wrote the boundary into the spec rather than only the PR body deliberately: the PR body
stops being read after merge, and this is precisely the mistake the metric names invite.

2. Defer the collector script - REBUTTED

The subtraction argues tiers 1-2 already close the declared gap and that "whether a real
collector accepts valid OTLP is the collector's contract."

Protobuf decodability and collector acceptance are different properties. Tier 2 proves our
bytes parse; it cannot show a collector's own validation admits them, and a payload that
decodes can still be rejected. The evidence that this distinction is not theoretical: the
first real-collector run failed on two defects tiers 1-2 could not see - the checksums asset
name 404s under the obvious guess (the real asset is named after the release repo and
distribution, not the archive), and reading the file exporter's output raced its flush. Both
were in the harness rather than the product, which is the point: a manual path nobody has
executed is not evidence, and this one was wrong until it ran.

Cost of keeping it is bounded and stated: no CI job invokes it, it is opt-in, checksum-
verified, refuses to fetch outside the pinned release, and refuses to execute an unverified
binary. It also produced the observation that became fix 3 below.

Its one automated hook, test_collector_script_is_present_and_pins_a_checksum, exists so the
documented path cannot silently rot into a broken reference or an unverified download; that
is a cheaper guard than deleting the tier and rediscovering the same two defects later.

3. A real finding hiding in the deferral - FIXED

Arguing point 2 exposed something worth having automated. The real-collector run showed
kirocrew.process.start_time correctly absent from the OTLP payload - it is a host-local,
reboot-unique token the JSONL exporter stamps per record, and it must never egress. That was
a manual observation in a script nobody runs, so it protected nothing.

Tier 2 now asserts it directly, and tier 1 already asserts the token IS present on the local
shard. Both halves are needed: either alone passes while the attribute sits on the wrong side
of the boundary.

4. Shrink the vendor half of the guide - PARTIALLY ACCEPTED

The drift argument is fair: vendor exporter options change with releases this repo does not
track, and unversioned vendor YAML in-repo is a maintenance claim.

Cutting the configs to links loses the thing that makes the page worth having - an operator
should not have to assemble a working collector config from three vendor sites to check
whether their metrics arrive. Instead the page now says plainly that the samples are
illustrative and not tracked, that each vendor's own documentation is authoritative, and
what this page actually owns and can keep correct: the two consent switches, the
full-signal-URL requirement, and the temporality preference.

If the vendor blocks do drift, deleting them is a one-line-per-block edit against a page that
now disclaims them, rather than a correctness bug.

Also this round

SAST (Semgrep) is green on 9483c7c34, confirming the round-2 placement fix. Design Review
PASS on the same SHA. GPT 5.6 reported "review incomplete" for d34a9b170 - a transient
non-verdict, not a finding; it is re-running.

Verification

• pytest test/metrics/ -q: 541 passed. The tier-2 test carrying the new assertion passes
  with the optional extra installed.
• flake8 / isort / mypy / black clean; black gate and docs-lint.sh pass.

No /ai-review override used.

[chenmingwei23]

Round 4 dispositions - 4f43d4015

Design Review: probes reach into private internals, tests verify against replicas - FIXED

This is the sharpest finding so far and it is correct. I built the drift-ratchet pattern for
two probes (the MCP _MANAGED_SERVER_NAMES reuse, the toggle anti-typo assertion) and then
did not apply it to the two other places the module reaches into a subsystem's private
representation. Worse, the failure mode is exactly the one this module claims to avoid: the
gap is CACHED, so a drifted probe goes dark permanently and looks identical to "never
ingested".

Both couplings now fail in the test rather than in production.

Knowledge. The fixture built the sources table by hand, so probe and test would have
kept agreeing after a schema rename. It is now built through the real KnowledgeStore with
add_source(), the pattern the existing knowledge tests use. The table name also moved out
of the inlined query into KNOWLEDGE_SOURCES_TABLE, so drift is detectable from either
side, and a new case asserts that constant against a schema the real store created.

Monitor loops. _FakeLoop / _FakeNudgeService are gone. The test now builds a real
AutoNudgeService(base_dir=tmp) holding real NudgeLoop objects, so renaming _loops or
active fails at construction. Construction alone touches no event loop (only start()
does), so this stays safe in a sync test. A separate case pins both names explicitly with a
diagnostic message.

Mutation-verified rather than assumed, because a ratchet that cannot fail is worse than none.
Renaming the probe's table constant to sources_renamed:

FAILED test_knowledge_probe_query_matches_the_real_stores_schema
       - KnowledgeStore creates no 'sources_renamed' table (has: [...])
FAILED test_knowledge_documents_counts_sources
       - sqlite3.OperationalError: no such table: sources_renamed
2 failed, 2 passed        # reverted: 36 passed

An earlier version of the schema case queried sqlite_master for a literal "sources" and
so did NOT fire on that mutation - it asserted the store's schema without asserting what the
probe reads. That is why the constant exists; the version above catches both directions.

The other four probes do not have this exposure, for the record: crons goes through cron's
own record helpers (a rename breaks the import, loudly), skills and lessons call public
methods, and config toggles are already covered by the resolution ratchet.

First Principles: framing - ALREADY FIXED, review read a stale description

This concern quotes the description saying "how many installs schedule crons... every question
about feature adoption is a manual survey" and correctly notes it contradicts the spec hunk's
operator-fleet scope. That contradiction was real, and it is what I fixed in round 3 - the
quoted sentences were removed from the description then, before this review ran. The verdict
appears to have been produced against the pre-edit body.

Verifiable now: gh pr view 7300 --json body | grep -c "feature adoption is a manual survey\|how many installs schedule crons" returns 0. The description leads with the
operator-drift problem and carries an explicit scope paragraph pointing at beacon.py.

No further change made, because the change this asks for is already in the branch. Flagging
the stale read rather than silently agreeing, since "the artifact cannot solve the problem the
description leads with" is no longer true of this description.

First Principles: shrink the vendor blocks - ACCEPTED (trimmed, not linked out)

Two reviewers have now raised this, and the sharpened version of the argument landed: after
round 3 added "illustrative, not tracked", the samples became content the page disowns while
still carrying sync cost. That is a fair reading of a half-measure.

Trimmed the part that actually drifts - option-level detail - while keeping a working shape:

• dropped resource_to_telemetry_conversion and its two-paragraph dimension/billing aside
• dropped the batch processor tuning and the Datadog site-region aside
• dropped the "Fanning out to several backends" block, which restated the pipeline shape
  already shown three times, reduced to one sentence

What remains per vendor is the exporter type, the credential-by-env reference, and the
pipeline wiring: the stable part. Exporter type names and the fact that Datadog wants delta
do not drift the way option schemas do, and this is the last thing a page about wiring up
OTLP can usefully keep.

I did not cut to links only. The page's value is that an operator can check whether their
metrics arrive without assembling a config from three vendor sites, and the stable
Kiro-Crew-side content it owns - two consent switches, the full-signal-URL requirement, the
temporality preference - is only useful next to a config that runs.

First Principles: the collector script cites no observed rejection - STANDS

Fair as stated: I have not observed a real collector reject our payload. The tier exists
because decodability and acceptance are different properties and only one of them was tested,
and it has already paid for itself twice by finding defects in its own path (the 404-ing
checksums asset name, the flush race) plus the non-egress observation that became an assertion
in round 3. Keeping it, opt-in and checksum-verified, with that limitation now stated plainly
here rather than implied.

Verification

• pytest test/metrics/ -q: 543 passed (36 in the inventory suite, +2 new ratchets).
• flake8 / isort / mypy / black clean; black gate and docs-lint.sh pass; local semgrep over
  the changed Python reports 0 findings.

No /ai-review override used.

[chenmingwei23]

Round 5 disposition - 1e90287bd

Inclusive Language - FIXED (self-inflicted by round 4)

The drift ratchet I added in round 4 read SQLite's schema catalog to list tables, and
sqlite_master contains a substring the gate flags. The table name is SQLite's own and
cannot be renamed, so the fix was to stop reading the catalog at all.

That turned out to be the better test anyway. Instead of asking "does the schema contain a
table named X", the ratchet now simply runs the probe against a database the real
KnowledgeStore built and asserts the count - which is exactly what the probe does in
production, and fails on a rename from either side without needing the catalog:

mutated KNOWLEDGE_SOURCES_TABLE -> "sources_renamed"
FAILED test_knowledge_documents_counts_sources
       - sqlite3.OperationalError: no such table: sources_renamed
1 failed, 2 passed        # reverted: 542 passed

With the catalog read gone, the separate schema case was redundant with the count case, so
the two are folded into one carrying the drift diagnostic in its assertion message. Net one
test fewer than round 4 (35 in the inventory suite, 542 overall) with the same detection - I
would rather say that plainly than leave a second case that only restated the first.

sqlite_schema, SQLite's modern alias, would also have satisfied the gate, but it needs
SQLite 3.33+ and this suite runs on Windows and two Python versions whose bundled SQLite I do
not control. Removing the dependency was the safer route.

Verification

• pytest test/metrics/ -q: 542 passed.
• flake8 / isort / mypy / black clean; black gate passes.
• Confirmed no flagged term remains in the diff.

No /ai-review override used.

[chenmingwei23]

Round 9 dispositions (head e18f40bf5, was b4c77f684)

The previous head was all-green with readiness: passed. First Principles raised an
advisory CONCERNS on it, and one of its two items is a factual error in my own
prose, so it earns a push rather than a reply.

FP: the "nothing proves serialization" premise is false -- FIXED in e18f40bf5

Verified, and FP is right. test/metrics/test_local_exporter.py is pre-existing on
main (from #5636), holds 15 tests, drives a real JsonlMetricExporter through a
PeriodicExportingMetricReader plus provider.force_flush(), and asserts valid
JSONL, shard rotation and prune behavior, and process identity stamped at resource
level. So "every test in this directory collects through an InMemoryMetricReader"
was simply untrue, and I wrote it in three places.

What survives the correction is a narrower and checkable claim. That file tests the
exporter ALONE: it constructs the exporter directly and feeds it from a provider
assembled inside the test. It never calls provider._build_recorder() (zero
occurrences), never asserts an instrument roster, and never asserts temporality. So
the gap the new tier closes is the same question asked of the LIVE build -- that the
roster the real build registers arrives, carrying the attribute values the modules
declare, at the temporality each instrument kind requires. That is also exactly
where FP put it.

Corrected in all three places, with no behavior change:

• test/metrics/test_otlp_wire_e2e.py module docstring -- the shipped one that
  mattered most, since it is the file's own statement of why it exists.
• the commit message.
• the PR description.

Gates re-run after the edit: test/metrics/ 608 passed, flake8, isort, black
baseline, docs-lint 252 files.

One correction back: FP counted 16 tests in that file; there are 15. Immaterial to
the finding, which stands.

FP: shrink the guide's vendor exporter blocks -- DECLINED, third ask

Same subtraction, third time, and the maintainer has already ruled on this file's
scope. The new wrinkle this round is that FP quotes the page's own disclaimer back
at it, so that part deserves an answer rather than a pointer.

The disclaimer is not evidence the blocks should go; it is the reason they are safe
to keep. The page draws the line itself, in the same paragraph FP quotes: vendor
options move with releases this repository does not follow, so each vendor's own
docs are authoritative and these blocks are a starting shape, while what the page
owns is the Kiro Crew side -- the two consent switches, the full-signal-URL
requirement, and the temporality preference. A reader is told, before the first
vendor block, exactly how much to trust it.

Deleting them costs the thing the guide exists for. An operator wiring an OTLP
endpoint for the first time needs to see one complete pipeline end to end; a
receiver stanza plus a link to three vendor sites does not show that, and the
unknown type: "awsemf" row in the troubleshooting table stops making sense
without the block it refers to. The blocks are also the shortest possible form
already: a receiver, one exporter, one pipeline.

Design (advisory, PASS): the election is guarded only by an AST test on one file

Accurate, and recorded rather than fixed. The AST ratchet is deliberately the
narrow guard: it pins that mark_install_reporter is called exactly once, from
run(), unconditionally, which is precisely the round 1 bug (the claim sat behind
_init_autonudge's KIROCREW_AUTONUDGE=0 early return). What it cannot pin is a
SECOND long-running entry point appearing later and claiming too, or claiming
instead.

That is the same cause the election itself is standing in for, and it has an exit
already named in the diff and the spec: install-scoped resource identity (#7266)
makes fleet dedup possible at query time, which is what lets the election retire.
Design's actual ask -- do not let the AST ratchet be the only thing that outlives
the election -- is a constraint on #7266's landing, not a change this PR can make,
and it is now on the record here alongside the spec text that describes the
boundary.

[buluoray]

Approved — 0 blocking, 3 non-blocking findings

Independent review of e18f40bf5, verified against source in a throwaway worktree rather than from the description. Every new execution path here is fail-closed: a probe that cannot answer yields a missing series plus a counted probe.failures increment, which is the exact confusion that counter exists to resolve. Nothing crashes, nothing exports a wrong number, nothing can break an export cycle. Nothing found justifies another blocking round.

The two things most likely to be quietly wrong were both right

Observable-counter monotonicity. kirocrew.inventory.probe.failures must report a monotonic cumulative total. The specific hazard I went looking for — a TTL-cached None re-incrementing on every collection, so one stuck probe inflates the count with no new failure and misreports the rate — does not occur: _ttl_cached returns a cache hit without reaching produce() or _note_probe_failure, and _probe_failures is increment-only outside the test reset. Corroborated by test_ttl_cache_caches_a_raised_failure_as_a_gap.

Concurrency under the PR's own stated two-reader condition. _lock is a real threading.Lock; every _cache access is guarded and the stored value is an atomic (expiry, value) tuple, while produce() deliberately runs outside the lock. Worst concurrent outcome on a cold entry is a double-produce (two SQLite opens), still monotonic — no torn read, no lost failure record, no deadlock. The auto-nudge claim also holds: list(service._loops.values()) is a single C-level construction that cannot observe a mid-iteration mutation, and _loops is mutated only on the loop thread.

Mutation-verified: 8 of 9 ratchets are genuinely falsifiable

Each mutation was byte-verified as landed before running pytest, to rule out no-op false greens.

Claim	Mutation	Result
Call-site ratchet	wrap in if; move to another method; call twice; delete	all four reddened test_reporter_claim_is_unconditional
Privacy ratchet	emit MCP server name as an attribute value	reddened at both the reader and the wire
Drift (knowledge)	wrong KNOWLEDGE_SOURCES_TABLE	reddened
Drift (MCP)	restate a managed server name locally	reddened
Anti-typo	rename a declared toggle to a nonexistent field	reddened
Series-shape	band a store-backed count (constant 1 + attribute)	reddened both halves
Instrument roster	unregister one gauge	reddened
Resource fidelity	drop service.name only on the wire	reddened
Write refusal	mode=ro → mode=rwc	reddened

The resource-fidelity assertion is genuinely dynamic, not a hardcoded list in disguise: I dropped the attribute on the serialization side only, leaving the live provider's Resource intact, and it caught it. A construction-side drop would have escaped precisely because both sides move together — which confirms the comparison is against the provider's own resource.

Also verified: the cron.py extraction is line-for-line behavior-preserving with the loadable half genuinely guarded (forcing (count, True) reddened two tests); enabled_count_from_disk has exactly the two claimed consumers; no code path registers inventory gauges while telemetry.enabled is off; the reporter claim sits at unguarded method-body indentation in run() after the _test_mode block closes; instrument parity between code and spec is exact (9 process + 9 inventory); docs-lint.sh passes; cron suite 2244 passed, metrics suite 606 passed.

---

Findings — all non-blocking, all in prose that describes code

1. The list_jobs timer rationale is factually wrong, and doubly unreachable. read_active_crons's docstring (src/kiro_crew/metrics/inventory_gauges.py:433-436) justifies reading from disk because "list_jobs re-arms the asyncio timer — calling that from a ticker thread with no running loop would raise, and worse, it cancels the existing timer first, which would stop every scheduled job."

Neither half survives current source. list_jobs (src/kiro_crew/cron.py:2901-2919) is cache-only — its body is return self._snapshot(list(self._jobs), include_disabled) and its own docstring says it performs no filesystem I/O; the _sync() → _load() → _arm_timer() path belongs to list_jobs_async. And _arm_timer (src/kiro_crew/cron.py:3081-3090) was already hardened for exactly this case: off-loop it takes except RuntimeError → loop = None → call_soon_threadsafe → return, cancelling nothing and raising nothing. Its inline comment names the blind-cancel hazard as the reason it was fixed.

This text is inherited, not invented — the same claim sits verbatim on main in count_enabled_from_disk's docstring, which this PR leaves untouched. What the PR does is propagate a condensed copy into a second file. The decision to read from disk stays correct for its other stated reasons (no process-global CronService handle, loadable is needed, drift avoidance). Worth fixing because this module's whole thesis is not trusting an inference that silently stops being true, and it now carries one about a neighbouring module. Ideally correct both sites.

2. The spec's pod paragraph is overstated for one of the nine gauges. docs/system-specs/modules/metrics.md:389-391 says a pod "boots with its own KIROCREW_HOME and copies no crons, sessions, or databases, so its inventory belongs to a different install and publishing it is correct." True for the stores it names — but mcp_discovery._mcp_sources() (src/kiro_crew/mcp_discovery.py:296-299) returns data_home()/"mcp.json" and Path.home()/".kiro"/"settings"/"mcp.json", and the second is machine-global. So host and pod publish overlapping readings for kirocrew.inventory.mcp.servers.

Not introduced here: agent.py:316, apps/bridges.py:216 and dashboard/handlers/mcp.py:86 all resolve that same host path, and kiro_home()'s own docstring names settings/mcp.json as a reader still pinned to host ~/.kiro. One qualifying sentence, no code change. Given how carefully the two-gateway limitation is documented in three places, this reads like an oversight rather than a position.

3. test_knowledge_probe_never_creates_the_database is fixture-backstopped, not probe-guarded. I could not falsify it by source mutation: even after removing the db_path.exists() short-circuit and switching to mode=rwc, it stayed green — sqlite3.connect will not create parent directories, and the fixture's tmp_path has no workspace/knowledge/. Its sibling write-refusal assertion is cleanly pinned, so the read-only mode is genuinely load-bearing; it is only this one assertion that cannot fail for the reason it exists. Making it falsifiable needs the fixture to pre-create the parent directory.

Two findings I would retire

Opus 4.8's Windows SQLite URI advisory does not hold on its stated mechanism. The probe's path is config_dir()/workspace/knowledge/knowledge.db (inventory_gauges.py:534) with fixed trailing literals, so no ? or # can appear — and ?/# truncation is precisely what the repo's pathname2url/as_uri() convention defends against (snapshot.py:611-617 documents it in those terms). Every cited precedent opens a user- or project-controlled path where the escape is load-bearing; this one does not. Failure would be fully degraded anyway (_ttl_cached catches, records a knowledge failure, caches None). At most a defense-in-depth consistency nit.

timeout=2.0 bounds lock-wait rather than query execution — fine for a COUNT(*) on the sources table, not worth a change.

---

Approving. The three findings are follow-up material: none is fail-open, two are pre-existing or inherited, and after nine bot rounds the only class left is cross-references to untouched files — which is the class the automated lanes structurally cannot see.