feat(metrics): identity, version, and environment resource attributes

[chenmingwei23]

What is the problem?

The MeterProvider resource carries only service.name. Nothing in an exported payload identifies the build that produced it, the install it came from, the process within that install, or the machine class it ran on:

• no service.version, so release-over-release comparison ("did the fix land") can only be done by comparing time ranges, which a gradual rollout muddies -- both versions report into the same buckets;
• no stable install identity, so any OTLP backend counting devices has nothing to count by. Worse than absent: when the key is omitted the SDK substitutes its own per-process uuid4, so every restart starts a brand-new series set and "this install over time" cannot be asked at all;
• no process identity either, so the several telemetry-enabled processes one install runs concurrently (gateway plus spawned agents/apps, the reason the local exporter shards per PID) would interleave their gauges and cumulative counters into one corrupted series;
• no host.cpu.logical_count, so kirocrew.process.cpu.seconds cannot be normalized into "percent of this machine" anywhere downstream -- the core count exists only client-side;
• no OS / arch / runtime attributes, so none of those work as fleet filters.

This is item 1 of #7232 and the first milestone slice of #7257.

Why this issue matters to the user

Any consumer of the OTLP egress seam (an edition-supplied collector, CloudWatch, Datadog, or the local Telemetry page reading the JSONL sink) inherits these labels on every series. Without them a fleet dashboard cannot group by version, count devices, separate concurrent processes, or normalize CPU -- and no downstream work can add them later, because resource attributes only exist if the client stamps them at export time.

How our fix solves it

_resource_attributes() is the single place the resource is built, and Resource.create() now consumes it. Every value is a closed set or an explicit clamp, because a resource attribute is a label on EVERY series this process exports -- one unbounded value here multiplies every instrument. Every probe fails soft: a failed read omits its attribute rather than losing telemetry or inventing a value.

Attribute	Value	Bound
service.instance.id	persisted anonymous install id (beacon.install_id)	one per install
process.pid	os.getpid()	one per live process
service.version	release-clamped via beacon.release	major.minor.patch, no build stamp
os.type	linux / darwin / windows	CLOSED, else other
host.arch	amd64 / arm64 / x86 (aliases folded)	CLOSED, else other
process.runtime.name	cpython / pypy / jython / ironpython	CLOSED, else other
process.runtime.version	beacon.python_minor()	major.minor, never the patch
host.cpu.logical_count	os.cpu_count()	small integer

Two identities, deliberately separate. service.instance.id counts DEVICES: a random UUID persisted on disk, never derived from hostname or username (which routinely embed an employee alias on a corporate desktop), stable across restarts. process.pid counts PROCESSES. Collapsing them would report one machine's 6-8 concurrent processes as 6-8 machines; omitting the pid would interleave those processes into one corrupted series. PID reuse across restarts reads as an ordinary counter reset downstream.

Where the id is minted, and why there. The probe is read-only (create=False: one stat plus a 32-byte read). The single write -- beacon.install_id(create=True), i.e. mkdir + mkstemp + link, race-safe per its own docstring, once per install ever -- sits in _build_recorder()'s live branch immediately before the resource is assembled. That placement is load-bearing in three ways:

1. No window. Because omitting the key yields the SDK's per-process uuid4 rather than an unlabelled resource, any gap before the id lands exports exactly the per-restart churn this attribute exists to prevent. Minting before the first build means no export can carry the substitute.
2. No mid-life swap. Acquiring the identity later would mean replacing a live resource, which a backend reads as a second, unrelated series set -- and swapping a healthy provider invalidates any recorder reference a caller already holds, silently dropping its observations. Minting up front makes both impossible by construction, and lets this module hold no backfill state at all.
3. Consent. The live branch is not reached when telemetry is off, so a disabled install creates nothing on disk.

The cost is one bounded write on a path that already runs synchronously on whatever thread touched telemetry first: the same first build pays KiroCrewConfig.load() (~14ms on a changed file) plus ~57ms of SDK import, both documented in docs/system-specs/modules/metrics.md. The mint is noise against that, and a failed mint costs only the label -- never the recorder.

Spec. The metrics spec documented the SDK Resource as the payload contract for both sinks but described only what must NOT egress on it. It now carries the attribute table, the two-identities rationale, why the install id may egress while the host-local kirocrew.process.start_time token may not, and the disclosed consequence that enabling metrics is what creates the install-id file even where the beacon itself is disabled.

Deliberately absent, documented in the spec and guarded by a regression test: the distribution channel (the beacon's data-minimization pass removed its channel field because channel sharply narrows the anonymity crowd a stable id hides in; re-adding it is a consent-inventory question, not a code convenience) and an install type (no reliable detection exists today; a guessed label would be confidently wrong).

Imports (beacon, __version__, platform) live at module scope per the top-level-imports rule -- stdlib-or-already-loaded, and no cycle, since the config loader itself imports kiro_crew.__version__ at module scope.

What tests we did

test/metrics/test_resource_attrs.py, 11 tests:

• identity / version / environment attrs present with clamped shapes (release-shaped version, major.minor runtime asserted against beacon.python_minor() so the two surfaces cannot drift, 32-hex install id, integer core count, process.pid == os.getpid());
• arch alias folding (x86_64/AMD64 -> amd64, aarch64 -> arm64);
• unknown OS / arch / runtime readings all fold to other (the closed-set guarantee, including process.runtime.name);
• the probe is read-only -- on a fresh home it omits the attribute and leaves the disk untouched -- while the build path mints;
• a fresh pre-enabled install (config on from first boot, beacon disabled) exports the id on its FIRST shard, and the exported value equals the minted id;
• a failed mint costs only the label: the recorder stays live and the resource falls back to the SDK's per-process uuid4, asserted by shape so it can never be mistaken for the persisted 32-hex id;
• a disabled install creates nothing on disk;
• version-probe failure omits only service.version; id-read failure omits only service.instance.id;
• channel / distribution / install_type stay absent;
• end-to-end: a live recorder's exported JSONL shard carries the resource attributes on the wire, not just in the dict.

Each behaviour is mutation-verified -- reverting the runtime closed set, or the build-path mint, turns the suite red rather than leaving a decorative assertion. Tests that assert on the install-id file are given their own KIROCREW_HOME, so a sibling test that mints cannot decide the outcome by execution order.

Gates: flake8 / isort / mypy clean on the changed files; the new test file is black-clean and provider.py stays on the black baseline (never whole-file reformatted); docs-lint.sh clean.

Any other suggestions on the work

• service.instance.id semantics are worth a second opinion before this merges. OTel semconv wants that key unique per instance, and Prometheus OTLP ingestion maps it to instance while demoting other resource attributes to target_info -- so at a Prometheus-family backend the gateway and its spawned agents still land on one (job, instance) pair despite process.pid. A composite ({install_id}-{pid}) plus a dedicated kirocrew.install.id for fleet GROUP BYs would satisfy both readings at no extra cardinality, since process.pid already turns the series set over per restart. It is a one-way door once dashboards key on the shape, which is why it is raised here rather than deferred.
• The privacy-adjacent coupling the spec now records -- enabling metrics mints the beacon's install-id file even where the beacon is disabled -- may belong in the telemetry-consent UI text, not only in the module spec. Out of scope for this PR.

Refs #7232, #7257 (milestone M0).

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of ccd18290ee8c47b3643016afe1ff9b74920f70e8 — 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 claims verified against the repo. Writing the review now.

First-Principles-Verdict: PASS

Client-stamped resource labels are the only way these fleet questions can ever be answered, and every attribute ships with a derived bound.

What this change ships

Intent: make every exported metric series identify the build, install, process, and machine class that produced it — an ADDITION (item 1 of #7232).

1. Every series now names its build version, release-clamped — justified
2. Series carry a stable anonymous install id; enabling metrics now creates that id file on disk even with the beacon off — declared, justified
3. Series carry the writing process's pid, separating one install's concurrent processes at a backend — justified
4. OS / arch / Python-runtime labels, folded to closed sets — justified
5. Core count travels with the payload so CPU seconds can be normalized downstream — justified
6. Distribution channel and install type deliberately absent, test-guarded — justified
7. Spec section documenting the resource contract — mandated (same-commit spec rule)

Every value reuses the beacon's existing clamps (beacon.release, beacon.python_minor, beacon.install_id — no second spelling; grepped, one definition each in beacon.py). The normalization target for the core count exists (kirocrew.process.cpu.seconds, metrics/process_gauges.py:79), and the egress seam exists (telemetry.otlp_endpoint, platform/defaults.py:492). No item is undeclared; the on-disk mint side effect is disclosed in both the description and the spec.

Watch

The description says the local Telemetry page "inherits these labels": the bytes land in the JSONL, but nothing in-repo reads them — the aggregator reads exactly one resource attribute, the start-time token (dashboard/handlers/telemetry.py:660). The only real consumers today are external OTLP backends.

Subtractions

• Drop the try/except around attrs["process.pid"] = int(os.getpid()) (provider.py:501-508) — os.getpid() cannot raise; the guard is unreachable surface the pragma: no cover itself admits.
• Shrink the ~75-line _resource_attributes docstring to the clamp rules; the two-identities and egress rationale is already the spec's job (docs/system-specs/modules/metrics.md §Resource attributes) and two prose copies will diverge.

[FIRST-PRINCIPLES-REVIEWED] ccd1829

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Diff reviewed: provider.py resource construction, spec section, and the new test suite, against the PR's stated problem and the author's own open question.

Design-Verdict: CONCERNS

The service.instance.id shape is an unresolved one-way door the author themself flags — settle it before dashboards key on it, not after.

Watch

• Install-scoped service.instance.id doesn't fully solve the stated interleave problem. The PR's own harm statement — concurrent processes "would interleave their gauges and cumulative counters into one corrupted series" — is only fixed at backends that key on the whole resource; at Prometheus-family OTLP ingestion (which maps service.instance.id → instance and demotes process.pid to target_info, as the description itself notes) the gateway and spawned agents still collapse into one (job, instance). The author's named alternative ({install_id}-{pid} composite + dedicated kirocrew.install.id) costs nothing extra since process.pid already churns the series per restart, and this is greenfield — deciding it now is cheap; after dashboards ship it's a series-set break. Decide in this PR rather than merging the ambiguity.
• Consent coupling: enabling metrics mints and egresses the beacon's install id even where the beacon is disabled — disclosed in the spec, but the consent-UI follow-up the author defers should be tracked as a real issue, not just a spec sentence.

Suggestions

• Adopt the composite-id shape now (it is strictly additive here and resolves the Watch item without new surface).

[DESIGN-REVIEWED] ccd1829

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No candidates were found by the discovery pass. I verified the production changes independently: beacon.release, python_minor, install_id(*, create=...), and UNKNOWN_VERSION all exist with matching signatures; the new module-level from kiro_crew import __version__, beacon introduces no import cycle (beacon imports none of the metrics package); every resource attribute is a closed set or clamped; each probe fails soft; and the mint sits only in the post-consent live branch. Nothing grounds a real defect on the changed lines.

No findings.

[OPUS-REVIEWED] ccd1829

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/metrics/provider.py:533 -- "service.instance.id" bypasses the metrics privacy guard and correlates OS/architecture with a stable install identity, contradicting the anti-fingerprinting contract -> Fix: remove the install-ID mint and resource attribute. (origin: validation)
[GPT-REVIEWED] ccd1829

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

[chenmingwei23]

First Principles CONCERNS on c60481cf0: subtraction ACCEPTED and implemented in a988810bf

The finding is correct, and the correction lands on a premise I wrote. Stating that
plainly because it is the useful part: the PR justified ~120 lines of backfill state
machine by claiming the recorder build path must stay free of blocking file I/O, and
that claim is contradicted by the same path.

I verified all three of the review's factual premises before deleting anything.

(a) The build path already blocks. _build_recorder (provider.py:562) calls
KiroCrewConfig.load() at line 574 -- a disk read -- and pays the OTEL SDK import
at ~line 590, both synchronously, on the first build. get_recorder's own comment
says so: "First build of the process: synchronous". So "no blocking file I/O on the
build path" was not a documented invariant being honoured. It was a rule this PR
introduced, and the backfill machinery was the patch for the gap that rule created.
That is the reviewer's central point and it holds.

(b) create=True is not more expensive than create=False on a non-fresh
install. beacon.py:374-385: both spellings take path.exists() ->
_read_state -> _valid_id -> return. The create flag only changes behaviour
when the file is absent or corrupt, where the mint is mkdir + mkstemp + link, once
per install lifetime. So the shipped create=False read and a create=True mint
cost the same on every install that has already minted.

(c) The mint's result must be discarded, and this is the part worth being careful
about. install_id's failure path returns _IN_MEMORY_ID -- a per-process uuid
(beacon.py:192, returned at :401 and :412 when create=True). Stamping that
into service.instance.id would give every restart a new identity and churn the
series, which is worse than omitting the attribute. So the new call ignores its
return value and the existing create=False read still sources the attribute; that
read returns "" on failure, so a read-only filesystem degrades to the attribute
being absent, which is the correct outcome and is unchanged.

---

What shipped in a988810bf

The mint sits in _build_recorder's live path, after the consent gate and before
the availability checks -- so it runs only when telemetry is actually enabled, never
on a host that has opted out.

Deleted: the globals _id_backfill_pending and _id_backfill_attempted, the
_Build.needs_id_backfill field and its docstring paragraph, _id_backfill_due(),
the backfill/hot-swap branch in _consent_worker (including its own mint call), both
_id_backfill_due() conditions on get_recorder's fast path, the backfill arming in
_install_locked, the backfill globals in reset_for_testing, and the two backfill
tests.

Measured effect on the PR's own diff, which is the number that matters rather than
the churn:

head	diff vs main
c60481cf0	520 insertions, 21 deletions
a988810bf	379 insertions, 10 deletions

Net between the two heads: 71 insertions against 201 deletions, so -130 lines,
in the range the review predicted. grep -rn '_id_backfill_pending\|_id_backfill_attempted\|_id_backfill_due\|needs_id_backfill' src/ test/ is empty.

Verification. One new test replaces the two deleted ones: on a fresh install
with consent enabled, building the recorder mints the id and the exported resource
attributes carry service.instance.id. Mutation-verified rather than assumed --
commenting out the create=True call turns it RED with AssertionError: build did not mint the install id, and restoring turns it green.

• pytest test/metrics/test_resource_attrs.py -> 9 passed
• pytest test/test_beacon.py -> 152 passed
• pytest test/metrics/test_provider.py -> 34 passed (the consent-change and
  rebuild paths, since _consent_worker and get_recorder were both edited)
• flake8, isort clean. black --check flags provider.py, which is pre-existing:
  the file is in .github/black-baseline.txt (line 338) and origin/main's own copy
  fails identically.

---

On why this was taken now rather than deferred. The usual reason to answer an
advisory concern in-thread instead of pushing is that a push re-rolls five
non-deterministic review lanes on a converged PR. That argument does not apply here:
this PR is already blocked on a main-owned red (the log-site census slack, fixed by
#7492) and has to be rebased once that lands, so the re-roll was already going to be
paid. Deferring would have meant shipping 120 lines and deleting them in a follow-up
-- paying the review cost twice for a net-negative diff.

The one thing I would flag for a reviewer of the new shape: the first synchronous
recorder build can now perform a write (three syscalls, once per install
lifetime) where before it only read. That is deliberate, it is inside install_id's
own try/except, and its failure degrades to the attribute being omitted rather than
to a wrong value or an exception on the build path.

[chenmingwei23]

Correction: the subtraction I accepted is REVERTED in 10085e3e6. The backfill architecture is right.

I accepted the First Principles subtraction, implemented it, and it drew an
immediate BLOCKING from GPT on the same span. I went and settled which reviewer was
right instead of picking one, and the answer is that the original design was
correct. Reverting, with the evidence, because a reversal without it is just
coin-flipping between two bots.

10085e3e6 is byte-identical in tree to c60481cf0 (git diff --stat c60481cf0 HEAD is empty); only the SHA differs, because restoring the ref itself is not a
push shape I am permitted to make.

---

What GPT found, and it is correct

BLOCKING -- provider.py:572 -- install-ID creation blocks the event loop
Anchor: no-blocking-call-on-event-loop
Fix: Remove the synchronous mint from _build_recorder; keep this path read-only.

I checked reachability rather than assuming, with an AST pass over src/ for
get_recorder() calls lexically inside an async def body. Three, and the third
decides it:

kiro_crew/acp/client.py:3829          inside async def ensure_ready()
kiro_crew/heartbeat.py:229            inside async def _beat()
kiro_crew/metrics/http_metrics.py:237 inside async def route_latency_middleware()

_consent_worker's own docstring already says so in this file: "get_recorder() is
called on the event loop by the route-latency middleware for every request." So the
first synchronous build genuinely can run on the loop, and putting a mkdir + mkstemp

• write + link there is new blocking I/O on it.

Worth noting what does NOT settle this: test_no_blocking_call_on_loop.py passed on
the deleted-backfill head (19 passed). That gate is syntactic -- it flags blocking
calls lexically inside an async def -- so it cannot see through the sync
get_recorder -> _build_recorder hop. Its green was not evidence of safety, and I
am saying that explicitly because it would have been the convenient thing to cite.

---

Where the First Principles analysis holds, and where it breaks

It holds on cost, and I verified all three of its premises before deleting anything:
_build_recorder already pays KiroCrewConfig.load() (provider.py:574) and the OTEL
SDK import synchronously; install_id(create=True) and create=False are
cost-identical once the file exists (beacon.py:374-385); and the failure path returns
_IN_MEMORY_ID (beacon.py:192) so the mint's result must be discarded. All true.

It breaks on two things the cost argument does not reach.

1. The anchor is categorical, not a budget. "This path already spends 71ms, so
three more syscalls are noise" is a fair performance claim and an invalid
rule-compliance one. What the pre-existing config read and SDK import actually show
is that this path carries a pre-existing on-loop violation the syntactic gate
cannot see -- which is an argument for getting that work off the loop, not for adding
a write beside it.

2. Consent-gating forces the mint after the build, which forces a rebuild. This
is the part that makes the prescribed two-liner unbuildable, and it is structural
rather than a matter of taste. The mint must satisfy three constraints at once:

• off the loop -- per the above;
• after consent is confirmed -- minting an install id on an opted-out host is a
  privacy regression, and is exactly why create=False exists for kirocrew telemetry status. So it cannot move to import time, even though import is off the
  loop;
• before the resource attributes are frozen -- Resource.create(...) is
  evaluated once per provider, so an id that appears afterwards is not in the live
  series.

The only place satisfying the first two is the consent worker thread, which by
construction runs after a recorder has already been built. Satisfying the third
from there means rebuilding once when the id newly appears -- and "rebuild once when
the id newly appears" is needs_id_backfill + _id_backfill_due() + the hot-swap.
The machinery is not a patch for an invented rule; it is what those three
constraints leave.

I did look for a fourth option. Minting in the worker without the rebuild is
strictly worse than either design: on a fresh pre-enabled install where consent never
moves, the worker mints and nothing ever rebuilds, so service.instance.id never
reaches the series and the PR's headline claim quietly fails on exactly the install
it was written for.

---

Measured, so the reversal is not free of information

The subtraction did shrink the diff as predicted -- 520 insertions/21 deletions at
c60481cf0 against 379/10 with the backfill gone, a net -130 lines, in the range the
review estimated. The size criticism was accurate. It is the placement that the
constraints forbid, so the cost is real and being paid deliberately for loop
isolation and for not minting on an opted-out host.

Verified on the restored head: pytest test/metrics/test_resource_attrs.py -> 10
passed; pytest test/test_no_blocking_call_on_loop.py -> 19 passed; flake8 and isort
clean; one commit; ASCII message.

---

Disposition, stated plainly. First Principles' CONCERNS is rebutted on the
placement and accepted on the size: the backfill stays because no other location
satisfies off-loop + post-consent + pre-freeze, and GPT independently blocked the
prescribed alternative on a named anchor. If a maintainer wants the machinery leaner
within those constraints -- collapsing the two globals into one state enum, or
folding _id_backfill_due() into the existing recheck predicate -- that is a fair
ask and I will take it. What cannot move is the mint onto the build path.

My earlier comment on this PR accepting the subtraction is superseded by this one.

[chenmingwei23]

Round 7 dispositions -- rebased onto 7531b9423, three findings fixed

The four red Backend Tests shards on 10085e3e6 were main-owned, not this PR. The shard log said:

AssertionError: `_BASELINE_LOG_SITE_CENSUS` is now looser than the code --
lower or drop these: dashboard/handlers/files.py: 1 sites, census says 3

That census drift was fixed on main after this PR's run started (05:31Z). Coverage Gate was purely downstream of it (backend-test=failure -- failing closed), not an independent coverage finding. Rebased onto current main; test/test_security_posture.py is 47 passed locally on the new base. No part of main's fix was folded into this diff -- the rebase only takes a clean merge ref.

Since the rebase force-push re-rolls every review lane anyway, the three legitimate findings below went into the same amended commit rather than costing a separate round.

GPT 5.6 -- both findings real, both fixed

1. provider.py:1110 -- stale _id_backfill_pending spawns consent workers forever. Correct, and reachable. Traced: the withdrawal path sets _recorder = MetricsRecorder(None) and returns at if not consent without going through _install_locked, so the pending flag survives the withdrawal. _id_backfill_attempted is only spent inside the backfill branch, which requires consent is True, so it stays False. The predicate therefore stays True forever on an install that has telemetry off: every get_recorder() -- called on the event loop by the route-latency middleware for every request -- misses the fast path, takes _lock, and schedules a fresh consent worker that immediately returns. A permanent per-request lock trip plus continuous thread churn, on the one config where the answer is "do nothing".

Fixed as suggested: _id_backfill_due() now requires a live provider, which is what makes the predicate mean what the flag's own comment claims ("a LIVE recorder without an id exists"). _take_provider_locked() nulls _provider on both the withdrawal and shutdown() paths, so both disarm. The flags are deliberately left as the withdrawal found them; the predicate is the thing that has to be honest.

2. provider.py:525 -- unknown python_implementation() bypasses the closed set. Correct. The docstring promised os.type, host.arch and process.runtime.name/version were all CLOSED sets folding to _ATTR_OTHER, but process.runtime.name passed the reading straight through -- so the doc was a promise the code did not keep, and a patched or exotic interpreter could mint its own label on every series. Fixed with _KNOWN_RUNTIME_NAMES (the four spellings the stdlib documents platform.python_implementation() can return) folding everything else to _ATTR_OTHER.

Both fixes are mutation-verified: reverting either one turns test/metrics/test_resource_attrs.py red (exit 1, 1 failed each), so neither test is decorative.

First Principles -- item 7 fixed, the Watch rebutted

Item 7, process.runtime.version duplicates beacon.python_minor() (beacon.py:308). Correct, fixed. Byte-identical logic with a docstring recording the same cardinality rule, i.e. one rule with two owners. Now calls the helper; import sys is gone from both the module and the test, and the test asserts against beacon.python_minor() so the two surfaces cannot drift.

The Watch (the backfill apparatus is oversized because the build path already pays a config read and a ~57 ms SDK import): rebutted, with an accepted residual. The premise measures the path's existing reads. The mint is a write -- mkdir + mkstemp + link -- and the two differ in the ways that matter here: a write blocks unboundedly on a slow or networked data dir where an import is served from page cache, and a write can fail, which is why install_id fail-softs to _IN_MEMORY_ID at all. Keeping the build path read-only is the fix an earlier round required for exactly this reason, on a path this module's own docstring says can be the event loop. Removing the state machine to mint synchronously would reopen it. The size of the apparatus is the deliberate, documented cost of that guarantee, and the alternative pays it in a worse currency. That existing reads are already expensive is an argument for making them cheaper, not for adding a write next to them.

Design Review -- both items escalated to the maintainer, no code change in this round

Both are advisory, and both ask for a decision about the exported shape rather than pointing at a defect, so I am not deciding them unilaterally on a PR whose author approves it.

1. service.instance.id uniqueness. I think this one is right and worth acting on. OTel semconv requires service.instance.id be unique per instance, and Prometheus OTLP ingestion maps it to instance while demoting other resource attributes to target_info -- so at a Prometheus-family backend the gateway and every spawned agent still collapse onto one (job, instance) pair, which is the exact collision process.pid was added to prevent. The proposed shape (service.instance.id = {install_id}-{pid}, plus kirocrew.install.id carrying the install identity for fleet GROUP BYs) satisfies both readings at zero extra cardinality: process.pid is already in the resource, so the series set already turns over per restart. Design Review is also right that it is a one-way door once dashboards key on the current shape, which argues for settling it before merge rather than after.

2. Consent coupling. Accurate as stated: with the beacon disabled, the metrics consent worker mints the beacon's persisted install id and ships it over any configured OTLP egress. The id is a random UUID never derived from hostname or username, and enabling metrics egress is itself the consent for metrics egress -- but "enabling metrics now creates a file the beacon owns" is a real new coupling between two consent surfaces, and this PR applied the consent-inventory argument to the channel field while leaving this one implicit. Flagging for the sign-off rather than arguing it away.

Gates on 8ac218687

pytest test/metrics/ 562 passed (random order, as CI runs it) - test/test_security_posture.py 47 passed - flake8 / isort / mypy clean - black gate passed with metrics/provider.py still in black-baseline.txt (never whole-file reformatted, so it did not graduate) - docs-lint.sh 251 files clean.

[chenmingwei23]

Round 8 -- took the subtraction; the blocking finding is gone structurally

Rebased onto 1ee69f225. provider.py goes from +303/-21 to +166/-1; the whole one-shot backfill apparatus is deleted rather than patched.

GPT 5.6 BLOCKING -- hot backfill invalidates active recorder references. Accepted, and removed rather than fixed.

The mechanism was real: replaced = _take_provider_locked(); _install_locked(built) then _flush_detached_provider(replaced) retires the old provider's readers, so a caller still holding the previous MetricsRecorder records into a dead provider and those observations are silently dropped. The distinction that made it this PR's problem rather than a pre-existing one: the consent-flip rebuild has the same shape, but there consent MOVED (losing the tail is the intent on withdrawal, and an opt-in has nothing to lose). The backfill swapped a healthy recorder purely to add a label, on a path where consent did not move -- a new occasion for loss that the PR introduced.

GPT's suggested fix was "defer identity backfill until a normal rebuild or restart". First Principles, independently, asked for the same thing more completely -- move beacon.install_id(create=True) into _build_recorder()'s live branch and delete the machine it obsoletes. Two lanes converging on one remedy, one of them blocking, is the answer. So the hot swap is not repaired: it no longer exists. _consent_worker is now byte-identical to main.

Deleted: _id_backfill_pending, _id_backfill_attempted, _Build.needs_id_backfill, _id_backfill_due(), the backfill/hot-swap branch in _consent_worker, the extra get_recorder() fast-path predicate, and the three backfill tests. The only surviving mention of the word is the docstring line stating the module holds no backfill state at all.

First Principles -- subtraction taken. This reverses my round-7 rebuttal, and it was right to.

Last round I argued the mint is a write while the path's existing ~14ms + ~57ms are reads, so the read cost did not license adding a write. That distinction does not survive contact with the numbers it cited: the ~57ms is a ~120-module SDK import, i.e. heavy file I/O on the same filesystem, so a cold or networked data dir makes the import the unbounded term and mkdir + mkstemp + link the noise. The spec citation (docs/system-specs/modules/metrics.md) was the stronger argument and I had it backwards.

And the shipped behaviour was worse than either review knew. Omitting service.instance.id does not produce an unlabelled resource -- the SDK substitutes its own per-process uuid4. Measured on this branch's pinned SDK:

Resource.create({"service.name": "kirocrew"}) ->
  service.instance.id: '82926241-d449-41c0-8f5f-bd759dab88f3'

So the old design's documented residual ("until one of those runs, the attribute is simply absent") was false. Every pre-enabled fresh install exported series labelled with a per-restart uuid4 until the backfill landed -- exactly the churn this attribute exists to prevent -- and the backfill then swapped the resource mid-life, which a backend reads as a second, unrelated series set. Minting before the first build removes the window entirely: there is no export the substitute can reach. The subtraction is therefore strictly better, not merely smaller. Docstring, commit message and spec now state the fallback accurately instead of claiming absence.

Consent is preserved by placement: the live branch is not reached when telemetry is off, so a disabled install still creates nothing. That is now a test.

Design Review -- spec ask done in this PR

Added a ### Resource attributes: what identifies a series (and what egresses) section to docs/system-specs/modules/metrics.md: the full attribute table with each value's bound, the two-identities rationale (install id counts devices, pid counts processes; collapsing them reports one machine's 6-8 processes as 6-8 machines), and the reasoning the spec was missing -- why the install id may egress on the Resource while kirocrew.process.start_time may not (the start-time token is a host-local process fingerprint with no fleet question to answer, so it stays on the JSONL line; the install id is random, not derived, answers "how many devices", and is the same id the beacon already sends). The consent coupling you flagged last round is documented there as a stated consequence: enabling metrics is what creates the install-id file, even with the beacon disabled.

On your second point -- the consent worker becoming a hand-rolled state machine -- this round removes the third branch rather than adding a fourth flag, so the worker is back to its two-branch shape.

Your round-7 ask for a composite service.instance.id ({install_id}-{pid}) plus a separate kirocrew.install.id did not recur this round. I have not treated that as settled and have not silently dropped it: it is a one-way door once dashboards key on the shape, so it is flagged for the maintainer's call rather than decided here.

One defect I found in my own new test

The regression test for the build-path mint passed under xdist while failing in isolation -- a sibling test mints the install id into the same conftest-shared home, so the assertion was decided by execution order rather than by the code. An order-dependent test that happens to pass is worse than no test. Fixed with a _own_home() helper that gives each id-sensitive test its own KIROCREW_HOME (config_dir() memoizes on that raw value, so it re-resolves) and asserts the home really is fresh. It now fails deterministically under the default parallel runner when the mint is removed.

Gates on 783b3371b

test/metrics/test_resource_attrs.py 11 passed; test/metrics/ 562 passed and test/test_security_posture.py 47 passed on this file content; flake8 / isort / mypy clean; black gate passed with metrics/provider.py still in black-baseline.txt (never whole-file reformatted); docs-lint.sh clean on 251 files. Each fix mutation-verified: reverting the runtime closed set, the earlier liveness gate, or the build-path mint turns the suite red.

[chenmingwei23]

Round 9 -- phantom description fixed; the mint placement is now a genuine deadlock between two lanes

Head ccd18290e. Three things this round, two of them straightforwardly mine.

Design Review + First Principles: phantom description. Correct, and entirely my fault.

Round 8 deleted the backfill machine and I updated the commit message and the docstrings but not the PR body, so the description still narrated a consent-worker mint, a one-shot backfill and a hot swap that no longer exist anywhere in the diff. Both lanes were right to flag it, and the rule I broke is the plain one: when the code shrinks, the description shrinks in the same push.

The body is rewritten to match the shipped code, in the repo's five-section shape rather than as a round-by-round narrative. Also fixed, both from the same cause:

• test_resource_attrs.py's module docstring still claimed "creation belongs to the consent-worker rebuild" while a test in the same file asserts the build path mints. Rewritten.
• The get_recorder() if-condition was left multi-line after its _id_backfill_due() term was removed -- residue of the deleted machine, exactly as Design Review read it. Collapsed back to one line, so the hunk is gone from the diff. provider.py is now +160/-1.

GPT 5.6 BLOCKING: synchronous install-id mint blocks the event loop

The mechanism is real and I am not disputing it: record_boot_to_ready() -> get_recorder() -> _build_recorder() can run on the loop, and the mint is mkdir + mkstemp + link.

But the remedy -- "remove the synchronous mint and keep this build path read-only" -- is the exact shape this same lane blocked one round ago, and the two lanes are now asking for opposite things. Three properties are in play and only two can hold at once:

	no write on the loop	no backfill state	no id-less window
shipped round 7	yes	no -- and the backfill's hot swap was GPT's round-8 BLOCKING (a retained recorder reference silently drops observations)	yes
GPT's remedy here	yes	yes	no -- a pre-enabled install's consent never moves, so nothing ever rebuilds and the id is never picked up for the whole process lifetime
shipped now	no -- one bounded write	yes	yes

The third column is not cosmetic. Omitting service.instance.id does not yield an unlabelled resource; the SDK substitutes a per-process uuid4 (measured on this branch's pinned SDK), so an "id-less window" is really a window of exactly the per-restart series churn this attribute exists to prevent.

Weighing what each corner costs: the current shape's cost is ~1ms of mkdir + mkstemp + link, once per install ever, on a path that already runs KiroCrewConfig.load() (~14ms on a changed file) plus ~57ms of SDK import synchronously on that same thread -- both documented in docs/system-specs/modules/metrics.md, and the ~57ms is itself a ~120-module read from the same filesystem, so it is the unbounded term here, not the mint. The other two corners each shipped a real defect: permanent uuid4 churn on pre-enabled installs, or silent metric loss on a provider swap.

There is a fourth corner that satisfies all three: mint the id at process startup, off the loop, leaving _build_recorder() read-only with no backfill state. Its price is that the metrics module's guarantee moves into its callers -- every process that can export (the gateway plus spawned agents) has to do it, and a process that forgets silently gets the SDK substitute back.

I am not resolving this by pushing a fourth variant of the same 10 lines, and I am not reaching for an override on my own judgement. Flagging for the maintainer's ruling. Nothing else on this head is blocking; the remaining lanes are advisory CONCERNS that this round addressed.

Gates on ccd18290e

test/metrics/test_resource_attrs.py 11 passed; flake8 / mypy clean; black gate passed with metrics/provider.py still on the baseline; docs-lint.sh clean. (The broad suite is CI's -- it is xdist-per-core and this box has no headroom.)