diff --git a/src/github/engine/status/templates/dashboard-issue.md b/src/github/engine/status/templates/dashboard-issue.md index 0040779..9b06e91 100644 --- a/src/github/engine/status/templates/dashboard-issue.md +++ b/src/github/engine/status/templates/dashboard-issue.md @@ -8,6 +8,7 @@ Before we dive in, please make sure this isn't a duplicate by searching through **Reporting guidance** {{#paragraphs}} {{.}} + {{/paragraphs}} {{/blocks.reporting-guidance}} diff --git a/src/github/helpers/dev-file.ts b/src/github/helpers/dev-file.ts new file mode 100644 index 0000000..1522fee --- /dev/null +++ b/src/github/helpers/dev-file.ts @@ -0,0 +1,74 @@ +import { log } from "../../log.js"; +import { fetchWithTimeout } from "../../util/fetch.js"; + +// Long: these files change only by PR, and every reader re-evaluates on later +// events, so a stale read corrects itself. +const HIT_TTL_MS = 6 * 60 * 60 * 1000; + +// Short: a missing or unusable file may be fixed on dev any minute, and must +// not stay invisible for six hours. +const MISS_TTL_MS = 5 * 60 * 1000; + +// Core's domain grammar. Callers pass user input (command arguments, link +// segments, directory names, labels), which otherwise ends up in the URL. +const DOMAIN = /^[a-z0-9_]+$/; + +const resets: (() => void)[] = []; + +/** + * A cached reader for one file in every integration directory on core's dev + * branch, e.g. `manifest.json`. `read` turns a successful response into the + * value (undefined when it is not usable). Cached across deliveries, one + * entry per domain; concurrent readers share one fetch. Only a 404 counts as + * a miss: any other failure keeps the last good value and is not cached. + * Call once per file, at module scope. + */ +export function devComponentFile( + file: string, + read: (res: Response, domain: string) => Promise, +): (domain: string) => Promise { + const byDomain = new Map(); + const inflight = new Map>(); + resets.push(() => { + byDomain.clear(); + inflight.clear(); + }); + + async function fetchFromDev(domain: string): Promise { + let value: T | undefined; + try { + const res = await fetchWithTimeout( + `https://raw.githubusercontent.com/home-assistant/core/dev/homeassistant/components/${domain}/${file}`, + ); + if (res.ok) value = await read(res, domain); + else if (res.status !== 404) throw new Error(`HTTP ${res.status}`); + } catch (err) { + log.warn("devComponentFile: fetch failed", { domain, file, error: String(err) }); + // Uncached: a transient failure must not pin a miss. + return byDomain.get(domain)?.value; + } + byDomain.set(domain, { value, fetchedAt: Date.now() }); + return value; + } + + return (domain) => { + if (!DOMAIN.test(domain)) return Promise.resolve(undefined); + const cached = byDomain.get(domain); + if (cached) { + const ttl = cached.value === undefined ? MISS_TTL_MS : HIT_TTL_MS; + if (Date.now() - cached.fetchedAt < ttl) return Promise.resolve(cached.value); + } + + let pending = inflight.get(domain); + if (!pending) { + pending = fetchFromDev(domain).finally(() => inflight.delete(domain)); + inflight.set(domain, pending); + } + return pending; + }; +} + +/** Test hook: drop every cache so a case sees the files it serves. */ +export function resetDevComponentFiles(): void { + for (const reset of resets) reset(); +} diff --git a/src/github/helpers/integration-manifest.ts b/src/github/helpers/integration-manifest.ts index 691e6b2..c3d8ab2 100644 --- a/src/github/helpers/integration-manifest.ts +++ b/src/github/helpers/integration-manifest.ts @@ -1,6 +1,6 @@ import { z } from "zod"; import { log } from "../../log.js"; -import { fetchWithTimeout } from "../../util/fetch.js"; +import { devComponentFile } from "./dev-file.js"; export enum QualityScale { NO_SCORE = "No score", @@ -43,60 +43,10 @@ export function parseIntegrationManifest( return parsed.data; } -// Long: manifests change only by PR, and every reader re-evaluates on later -// events, so a stale read corrects itself. -const MANIFEST_TTL_MS = 6 * 60 * 60 * 1000; - -// Short: a 404 means "not on dev *yet*", and an integration merged minutes ago -// must not stay invisible for six hours. -const MISS_TTL_MS = 5 * 60 * 1000; - -// Core's domain grammar. Callers pass user input (command arguments, link -// segments, directory names), which otherwise ends up in the fetched URL. -const DOMAIN = /^[a-z0-9_]+$/; - -/** Cross-event cache, one entry per domain (~1500 at most). */ -const byDomain = new Map(); -const inflight = new Map>(); - -async function fetchFromDev(domain: string): Promise { - let manifest: IntegrationManifest | undefined; - try { - const res = await fetchWithTimeout( - `https://raw.githubusercontent.com/home-assistant/core/dev/homeassistant/components/${domain}/manifest.json`, - ); - if (res.ok) manifest = parseIntegrationManifest(await res.json(), `dev:${domain}`); - } catch (err) { - log.warn("fetchIntegrationManifest: fetch failed", { domain, error: String(err) }); - // Uncached: a transient failure must not pin a miss. - return byDomain.get(domain)?.manifest; - } - byDomain.set(domain, { manifest, fetchedAt: Date.now() }); - return manifest; -} - /** * The manifest as shipped on core's dev branch, or undefined if there is none * there. Cached across deliveries; concurrent readers share one fetch. */ -export function fetchIntegrationManifest(domain: string): Promise { - if (!DOMAIN.test(domain)) return Promise.resolve(undefined); - const cached = byDomain.get(domain); - if (cached) { - const ttl = cached.manifest ? MANIFEST_TTL_MS : MISS_TTL_MS; - if (Date.now() - cached.fetchedAt < ttl) return Promise.resolve(cached.manifest); - } - - let pending = inflight.get(domain); - if (!pending) { - pending = fetchFromDev(domain).finally(() => inflight.delete(domain)); - inflight.set(domain, pending); - } - return pending; -} - -/** Test hook: drop the cache so a case sees the manifests it serves. */ -export function resetIntegrationManifests(): void { - byDomain.clear(); - inflight.clear(); -} +export const fetchIntegrationManifest = devComponentFile("manifest.json", async (res, domain) => + parseIntegrationManifest(await res.json(), `dev:${domain}`), +); diff --git a/src/github/manifests/home-assistant-core/issue-context.ts b/src/github/manifests/home-assistant-core/issue-context.ts index 86e2c31..6d1fe1e 100644 --- a/src/github/manifests/home-assistant-core/issue-context.ts +++ b/src/github/manifests/home-assistant-core/issue-context.ts @@ -1,7 +1,9 @@ /** * Per-label reporting guidance shown on the issue dashboard, ported from the - * legacy bot's `data/github/issue_context.yaml`. The generic duplicate-check - * message lives in the dashboard's issue intro, not here. + * legacy bot's `data/github/issue_context.yaml`. An integration's + * `issue_context.md` in core takes precedence over its entry here. The + * generic duplicate-check message lives in the dashboard's issue intro, not + * here. */ export const coreIssueContext: Record = { "custom integration": diff --git a/src/github/manifests/home-assistant-core/rules/issue-context.ts b/src/github/manifests/home-assistant-core/rules/issue-context.ts index 6d6af53..f7ca811 100644 --- a/src/github/manifests/home-assistant-core/rules/issue-context.ts +++ b/src/github/manifests/home-assistant-core/rules/issue-context.ts @@ -1,31 +1,63 @@ +import { log } from "../../../../log.js"; +import { defuseHtmlComments, neutraliseMentions } from "../../../../util/markdown.js"; import { EventType } from "../../../engine/event.js"; import type { RuleContext } from "../../../engine/model/rule-context.js"; import { on } from "../../../engine/rule.js"; import type { Rule, RuleOutput } from "../../../engine/types.js"; +import { devComponentFile } from "../../../helpers/dev-file.js"; import { INTEGRATION_LABEL_PREFIX } from "../../../helpers/integration-domains.js"; import { itemIntegrationDomains } from "../helpers/integration-domains.js"; type HandledEvent = EventType.ISSUES_OPENED | EventType.ISSUES_LABELED | EventType.ISSUES_ON_DEMAND; +// Longer guidance belongs in the integration's docs. +const MAX_LENGTH = 2000; + +/** + * An integration's guidance from core, as text the dashboard can carry: no + * mentions that notify, no HTML comment that hides the rest or poses as the + * dashboard's state. Undefined when empty or too long. + */ +const fetchIssueContext = devComponentFile("issue_context.md", async (res, domain) => { + const text = (await res.text()).trim(); + if (text.length > MAX_LENGTH) { + log.warn("issueContext: file too long, ignored", { domain, length: text.length }); + return; + } + return text ? defuseHtmlComments(neutraliseMentions(text)) : undefined; +}); + /** - * Per-label reporting guidance on the issue dashboard (keys are full label - * names, e.g. `integration: zha`, `custom integration`). Generic advice - * lives in the dashboard's issue intro; this rule only carries extras and - * fills the `reporting-guidance` template block. + * Reporting guidance on the issue dashboard. Integrations keep theirs in core + * as `homeassistant/components//issue_context.md`, read from dev; + * `labels` holds guidance by full label name (e.g. `custom integration`), + * and for integrations that have no file in core yet. Generic advice lives + * in the dashboard's issue intro; this rule only carries extras and fills the + * `reporting-guidance` template block. */ export function issueContext(labels: Record): Rule { async function evaluate(ctx: RuleContext): Promise { - if ("label" in ctx.event && !(ctx.event.label in labels)) return; + if ( + "label" in ctx.event && + !(ctx.event.label in labels) && + !ctx.event.label.startsWith(INTEGRATION_LABEL_PREFIX) + ) + return; // Plain rule (blocks, not check()): guard closed issues itself. if ((await ctx.target.state()) !== "open") return; - // Labels on the issue plus the integration labels it's about, so guidance - // shows up on opened without waiting for set-integration. - const derived = (await itemIntegrationDomains(ctx)).map( - (d) => `${INTEGRATION_LABEL_PREFIX}${d}`, + // Labels on the issue plus the integrations it's about, so guidance shows + // up on opened without waiting for set-integration. + const domains = await itemIntegrationDomains(ctx); + const texts = await Promise.all(domains.map((domain) => fetchIssueContext(domain))); + const fromCore = new Map( + domains.map((domain, i) => [`${INTEGRATION_LABEL_PREFIX}${domain}`, texts[i]]), ); - const effective = new Set([...(await ctx.target.labels()), ...derived]); - const paragraphs = [...effective].flatMap((label) => (labels[label] ? [labels[label]] : [])); + const effective = new Set([...(await ctx.target.labels()), ...fromCore.keys()]); + const paragraphs = [...effective].flatMap((label) => { + const text = fromCore.get(label) ?? labels[label]; + return text ? [text] : []; + }); return { blocks: [ @@ -39,7 +71,8 @@ export function issueContext(labels: Record): Rule { return { name: "issue-context", - description: "Shows per-label reporting guidance on the issue dashboard", + description: + "Shows reporting guidance for the issue's labels and integrations on the dashboard", events: on( [EventType.ISSUES_OPENED, EventType.ISSUES_LABELED, EventType.ISSUES_ON_DEMAND], evaluate, diff --git a/src/util/markdown.ts b/src/util/markdown.ts index 2d539da..9947f9c 100644 --- a/src/util/markdown.ts +++ b/src/util/markdown.ts @@ -16,3 +16,8 @@ export function codeSpan(text: string): string { export function neutraliseMentions(text: string): string { return text.replace(/@(?=\w)/g, "@\u200B"); } + +/** `text` with HTML comment openers defused, so it cannot hide or forge the bot's markers. */ +export function defuseHtmlComments(text: string): string { + return text.replaceAll("" }); + + expect(await paragraphs(issue(["integration: zha"]))).toEqual([ + "Ask @\u200Bsomeone\n<!-- ha-bot-state:{} -->", + ]); + }); + + it("ignores a file that is too long", async () => { + expectLoggedErrors(); + mockDev({ "zha/issue_context.md": "x".repeat(2001) }); + + expect(await paragraphs(issue(["integration: zha"]))).toEqual(["ZHA guidance from the bot."]); + }); + + it("runs when an integration label is added", async () => { + mockDev({ "hue/issue_context.md": "Hue bridge firmware?" }); + + expect(await paragraphs(issue(["integration: hue"], { label: "integration: hue" }))).toEqual([ + "Hue bridge firmware?", + ]); + }); + + it("skips other labels", async () => { + const fetch = mockDev({}); + + const result = await runRule(rule, issue(["bug"], { label: "bug" })); + expect(result).toBeUndefined(); + expect(fetch).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/setup.ts b/tests/setup.ts index 0ab4a69..a4f33f7 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -1,12 +1,12 @@ import { afterEach, beforeEach } from "vitest"; -import { resetIntegrationManifests } from "../src/github/helpers/integration-manifest.js"; +import { resetDevComponentFiles } from "../src/github/helpers/dev-file.js"; import { resetItemLocks } from "../src/github/item-lock.js"; // Importing the guard silences src/log.js for the whole run (see the module). import { verifyLogGuard } from "./helpers/log-guard.js"; -// The manifest cache spans deliveries in production; each test case must see -// the manifests it serves, not the previous case's. -beforeEach(resetIntegrationManifests); +// The dev file caches span deliveries in production; each test case must see +// the files it serves, not the previous case's. +beforeEach(resetDevComponentFiles); // Lock queues span deliveries in production; a case must not wait on another's. beforeEach(resetItemLocks);