From d2e36e2546fa658781181886b028051852d01acd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 18 Jul 2026 08:06:42 +0200 Subject: [PATCH] fix(mcp): exempt the reviewed discovery allowlist from the mcp:false veto MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The signal registry's `mcp:false` flag vetoes a metric-status resolution so an unreviewed non-exposed signal cannot leak through the id/display-name path. The metric-status discovery allowlist is a deliberate, human-named exposure — co-equal to `mcp:true` — so its ids must stay resolvable, or compare_metric / get_metric_baseline would advertise a metric in discovery that then refuses to resolve. Exempt the allowlist from the veto; the guard still fires for genuinely-unreviewed mcp:false ids (workout HR stats). --- src/lib/mcp/__tests__/rich-reads.test.ts | 37 ++++++++++++++++++------ src/lib/mcp/rich-reads.ts | 29 +++++++++++++++++-- 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/lib/mcp/__tests__/rich-reads.test.ts b/src/lib/mcp/__tests__/rich-reads.test.ts index f60b47b95..f786b14cc 100644 --- a/src/lib/mcp/__tests__/rich-reads.test.ts +++ b/src/lib/mcp/__tests__/rich-reads.test.ts @@ -133,16 +133,35 @@ describe("resolveRichMetric", () => { // v1.30.4 (C2) — the signal registry's `surfaces.mcp` flag is documented as // the SINGLE source of truth for MCP exposure, but `resolveRichMetric`'s // metric-status-registry path (exact id / display-name match) used to - // resolve an id regardless of that flag. `CARDIO_RECOVERY` / - // `WRIST_TEMPERATURE` / `SLEEP_SCORE` are all valid `METRIC_STATUS_IDS` - // entries but carry `mcp:false` in the signal registry — pin that they - // now stay unreachable, closing the contract gap (no PHI actually leaked - // through it, but a future `mcp:false` signal without this guard would). + // resolve an id regardless of that flag. `AVERAGE_HEART_RATE` / + // `MAX_HEART_RATE` are valid `METRIC_STATUS_IDS` entries but carry + // `mcp:false` in the signal registry AND are NOT on the reviewed + // metric-status discovery allowlist — pin that they stay unreachable, + // closing the contract gap (no PHI actually leaked through it, but a future + // `mcp:false` signal without this guard would). The reviewed discovery ids + // (`CARDIO_RECOVERY` / `WRIST_TEMPERATURE` / `SLEEP_SCORE` / …) are the one + // deliberate exemption and stay resolvable — asserted below. it("honours the signal registry's mcp:false as a hard veto over a metric-status hit", () => { - expect(resolveRichMetric("CARDIO_RECOVERY")).toBeNull(); - expect(resolveRichMetric("cardio recovery")).toBeNull(); - expect(resolveRichMetric("WRIST_TEMPERATURE")).toBeNull(); - expect(resolveRichMetric("SLEEP_SCORE")).toBeNull(); + expect(resolveRichMetric("AVERAGE_HEART_RATE")).toBeNull(); + expect(resolveRichMetric("MAX_HEART_RATE")).toBeNull(); + expect(resolveRichMetric("max heart rate")).toBeNull(); + }); + + // v1.30.4 (coverage review) — the reviewed metric-status discovery allowlist + // is a deliberate MCP exposure, co-equal to a signal `mcp:true`. A + // `mcp:false` on the same signal must NOT strip these ids' resolvability, or + // `compare_metric` / `get_metric_baseline` would advertise a metric in + // discovery that then refuses to resolve. + it("exempts the reviewed metric-status discovery allowlist from the mcp:false veto", () => { + expect(resolveRichMetric("CARDIO_RECOVERY")?.measurementType).toBe( + "CARDIO_RECOVERY", + ); + expect(resolveRichMetric("WRIST_TEMPERATURE")?.measurementType).toBe( + "WRIST_TEMPERATURE", + ); + expect(resolveRichMetric("SLEEP_SCORE")?.measurementType).toBe( + "SLEEP_SCORE", + ); }); it("still resolves a metric-status id the signal registry does NOT mark mcp:false", () => { diff --git a/src/lib/mcp/rich-reads.ts b/src/lib/mcp/rich-reads.ts index 73eb7016d..0ceeb0c70 100644 --- a/src/lib/mcp/rich-reads.ts +++ b/src/lib/mcp/rich-reads.ts @@ -214,9 +214,19 @@ function fromRegistry(id: string): RichMetric | null { // the SAME id in the signal registry as a hard veto over a metric-status // hit; an id absent from the signal registry (most of `METRIC_STATUS_IDS`) // is unaffected — metric-status stays authoritative when the two don't - // disagree. + // disagree. The one exemption is the reviewed metric-status discovery + // allowlist (`REVIEWED_DISCOVERY_IDS`): those ids are a deliberate, + // human-named MCP exposure — co-equal to a signal `mcp:true` — so a + // `mcp:false` on the same signal must NOT strip their resolvability, or + // `compare_metric` / `get_metric_baseline` would advertise a metric in + // discovery that then refuses to resolve. const sig = getSignal(id); - if (sig && sig.kind === "measurement" && sig.surfaces.mcp === false) { + if ( + sig && + sig.kind === "measurement" && + sig.surfaces.mcp === false && + !REVIEWED_DISCOVERY_IDS.has(id) + ) { return null; } const metric: RichMetric = { @@ -400,6 +410,21 @@ const METRIC_STATUS_DISCOVERY_IDS = [ "ENERGY_EXPENDITURE_KJ", ] as const satisfies readonly MetricStatusMetricId[]; +/** + * The reviewed metric-status discovery allowlist as a membership Set. Each + * member is a DELIBERATE, human-named MCP exposure (v1.30.4 coverage review) — + * co-equal to a signal-registry `mcp:true`, just carried in the metric-status + * layer instead of the signal layer. `fromRegistry`'s `mcp:false` veto exempts + * these ids so a reviewed metric stays resolvable (`compare_metric` / + * `get_metric_baseline` / `detect_changepoints`); the veto still fires for any + * UNREVIEWED `mcp:false` signal that happens to carry a metric-status entry + * (the two mental-health screeners and every `ENV_*` signal are absent from + * this list and have no metric-status entry, so they stay unreachable). + */ +const REVIEWED_DISCOVERY_IDS: ReadonlySet = new Set( + METRIC_STATUS_DISCOVERY_IDS, +); + /** * The metric-status-only ids `list_metrics` / the inventory resource / * `search` can now discover: the registry id (also the `metric:` id `fetch`