From b5d3336aacc4fd2c8c79ec67ac40977bcb1c7340 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 21:43:16 -0500 Subject: [PATCH 1/2] fix(report): keep a clean check clean under the issue channel, and deliver a long report Two defects in the issue channels of private-report. A converged check reported drift and wrote. The marker label was declared for every target an issue channel reports on, so a repository that never held the label showed it as missing: a check answered drift and exit 1, and issue-on-failure then created the label and a report issue. The channel now reads the marker label before the target's work and declares it unless the answer is 404. A repository without the label plans nothing, and one that holds it keeps it, in check and in apply. A read that fails any other way declares the label, as every target did before, so an unread label is never deleted. An apply no longer creates the label on a healthy repository, and the issue channels cost one more GET per private target per run. A report over GitHub's 65536-character issue-body limit earned a 422 on every run. The issue channel now composes its report under that limit: lines that do not fit are left out, section detail kept ahead of the transcript that repeats it, and the body says how many lines were left out and marks each cut. The artifact channel still delivers the whole report. The e2e mock serves the marker read for a delivery target, and the fuzzer lets a raw target receive it, since the read precedes the parse. --- docs/operate/private-repositories.md | 13 +- src/flows/deliver.ts | 10 +- src/report/composer.ts | 111 ++++++++-- src/report/delivery.ts | 53 +++-- src/report/issue-report.ts | 30 +++ test/e2e/fuzz.ts | 8 +- test/e2e/mock/chaos.ts | 1 + test/e2e/mock/core-paths.ts | 40 +++- ...-report-issue-on-failure-healthy-quiet.yml | 25 +-- test/flows/deliver.test.ts | 74 ++++--- test/flows/multi.test.ts | 194 ++++++++++++++++-- test/report/composer.test.ts | 117 +++++++++++ test/report/delivery.test.ts | 23 +++ test/report/issue-report.test.ts | 28 +++ 14 files changed, 629 insertions(+), 98 deletions(-) diff --git a/docs/operate/private-repositories.md b/docs/operate/private-repositories.md index fb33e9716..8f1e5598a 100644 --- a/docs/operate/private-repositories.md +++ b/docs/operate/private-repositories.md @@ -87,24 +87,23 @@ Every other channel follows the same four rules: - **Which issue:** found by the marker label, or by its exact title when someone removed the label. Either way it must be one of the action's own reports: the body carries the report heading. A same-titled issue you opened by hand is left alone, unless its body pastes a report verbatim. - **Whose issue:** the creator does not matter, so a rotated PAT reuses the issue instead of opening a second one. With several reports, an open one wins, then the newest. - **Each run:** the body is replaced; the issue opens when the target fails or drifts and closes when it is healthy. +- **A long report is cut to fit:** GitHub keeps at most 65,536 characters in an issue body. A longer report keeps every section's status and leaves lines out: from the transcript first, then from the sections with the most detail. It opens by saying how many lines were left out, and each cut is marked in place. The `artifact` channel delivers the whole report. +- **The marker label:** the first delivery creates `settings-as-code-report`. Each run reads it once per target, before that target's settings are checked or applied. On a repository that holds it, a declared `labels` section manages it like any declared label, so it is not deleted as undeclared. On a repository without it, nothing is added. A read that fails counts as held, so the label is never deleted unread. - **What the log says:** every write is announced with the placeholder, never the slug: `report: updated issue #7 in private repository #1` (or `created`), and `report: created label "settings-as-code-report" in private repository #1` when the delivery creates the marker label. A write that landed before a later step failed is announced the same way, ahead of the warning. - **Permission:** the PAT needs `"Issues"` (read and write) on every target repository, on top of the section permissions. `private-report: issue-on-failure` is the quiet variant of `issue`. When a target fails (or drifts in check mode) it behaves identically: the reused, marker-labelled report issue is written and opened. -On a healthy run it only looks up an open report issue: +On a healthy run it only reads: the marker label, then an open report issue carrying it: - **One is open,** left over from an earlier failure: it is updated with the healthy report and closed. - **None is open:** nothing is written, no issue, no label, zero notification noise, and the log says so: `report: nothing to deliver for private repository #1`. -A repository that never fails never sees an issue, at the cost that healthy runs' reports are not delivered anywhere. Choose `issue` or `artifact` when you need the report mirror on every run. +A repository that never fails never sees an issue or the marker label: both appear only when a report is delivered. The cost is that healthy runs' reports are not delivered anywhere. Choose `issue` or `artifact` when you need the report mirror on every run. -Two caveats on the quiet variant: +One caveat on the quiet variant: a removed marker label hides an open issue. If someone removes the label while a report issue is open, the healthy path cannot find the issue, so it stays open until the next failing run recreates the label and reclaims it. -- **A declared `labels` section still gets the marker.** The `settings-as-code-report` marker label is injected into it, so the apply creates the label even on healthy repositories (a label, not a notification). -- **A removed marker label hides an open issue.** If someone removes the label while a report issue is open, the healthy path cannot find the issue, so it stays open until the next failing run recreates the label and reclaims it. - -Everything else matches `issue`: the same delivery gate, the same Issues grant (the healthy lookup is a read; the open and close are writes), and delivery still runs in `mode: check`. +Everything else matches `issue`: the same delivery gate, the same Issues grant (a healthy run only reads; the open and close are writes), and delivery still runs in `mode: check`. `private-report: artifact` concatenates the report for every proven-private target into one document, encrypts it to an age recipient, and uploads it as the workflow artifact `settings-as-code-private-report` (file `private-report.md.age`). Use it when the people who need the report cannot be given repository access, since the archive travels with the run rather than living in the target repo. diff --git a/src/flows/deliver.ts b/src/flows/deliver.ts index adfcf444c..116063ef4 100644 --- a/src/flows/deliver.ts +++ b/src/flows/deliver.ts @@ -21,7 +21,6 @@ import { describeProblem, type Problem } from "../problem.js"; import type { ArtifactUploader } from "../report/artifact-report.js"; import { type ClosedOutcome, - isIssueChannel, openReportChannel, type PrivateReportChannel, type RedactedDetail, @@ -137,8 +136,9 @@ export interface OpenedTarget { export interface Delivery { /** - * `work` is told whether to inject the issue channel's marker label. On close, a redacted target proven private or - * internal gets its report, an unproven one the withheld notice, and its public line is closed-value only. + * `work` is told whether to inject the issue channel's marker label, which the channel answers for a target it will + * report on. On close, a redacted target proven private or internal gets its report, an unproven one the withheld + * notice, and its public line is closed-value only. */ target( opened: OpenedTarget, @@ -192,7 +192,9 @@ export async function withDelivery( reports !== null && exposure.kind === "redacted" && isPrivateVisibility(exposure.visibility); const delivery: Delivery = { async target({ repo, channel, exposure }, work) { - const outcome = await work(deliverable(exposure) && isIssueChannel(cfg.privateReport)); + const declaresMarker = + reports !== null && deliverable(exposure) && (await reports.declaresMarker(repo)); + const outcome = await work(declaresMarker); return closeTarget(io, channel, outcome, async (detail) => { if (reports === null) { return; diff --git a/src/report/composer.ts b/src/report/composer.ts index 60c742100..e78d7c7f9 100644 --- a/src/report/composer.ts +++ b/src/report/composer.ts @@ -1,6 +1,7 @@ /** - * One markdown document per redacted target, rendered identically for both channels. Pure, and structurally typed on - * purpose, so it depends on no action-layer types. + * One markdown document per redacted target, rendered identically for both channels, except that a channel whose + * document has a length limit gets it cut to fit. Pure, and structurally typed on purpose, so it depends on no + * action-layer types. */ import type { AnnotationLevel } from "../io.js"; @@ -29,6 +30,9 @@ export interface ReportInput { transcript: TranscriptLine[]; } +/** Joins the lines of one detail cell: a table row is a single line, so the cell breaks its lines in HTML. */ +const BREAK = "
"; + /** A fence longer than any backtick run inside the content, so a transcript line can never terminate the block early. */ function fenceFor(content: string): string { const longest = content.match(/`+/g)?.reduce((max, run) => Math.max(max, run.length), 0) ?? 0; @@ -42,12 +46,34 @@ function transcriptLine(entry: TranscriptLine): string { /** The report's first line. src/report/issue-report.ts reads it back as the proof that an issue body is one of these reports. */ export const REPORT_HEADING = "# settings-as-code private report:"; -export function composeReport(input: ReportInput): string { +/** How many lines of each list a document carries: one count per section's detail, in outcome order, and the transcript's. */ +interface Kept { + detail: number[]; + transcript: number; +} + +/** Stands where a list was cut, in a detail cell and under the transcript alike. */ +function leftOutMark(count: number): string { + return `(lines left out: ${count})`; +} + +/** The document carrying `kept` lines of each list. A list cut short ends in its mark, and any cut is announced under the heading. */ +function render(input: ReportInput, kept: Kept, limit: number): string { + const total = input.outcomes.reduce((sum, o) => sum + o.detail.length, input.transcript.length); + const carried = kept.detail.reduce((sum, count) => sum + count, kept.transcript); const lines: string[] = [ `${REPORT_HEADING} ${input.target}`, "", "Full, unredacted report for this target. The public run redacts it; this document is its private mirror.", "", + ]; + if (carried < total) { + lines.push( + `**Cut to fit ${limit} characters.** Lines left out: ${total - carried} of ${total}; each cut is marked where it was made.`, + "", + ); + } + lines.push( renderTable("| | |\n|---|---|", [ ["Target", markdownCell(input.target)], ["Admin repository", markdownCell(input.adminRepo)], @@ -59,18 +85,22 @@ export function composeReport(input: ReportInput): string { "", "## Sections", "", - ]; + ); if (input.outcomes.length === 0) { lines.push("No sections ran for this target."); } else { lines.push( renderTable( "| Section | Status | Detail |\n|---|---|---|", - input.outcomes.map((outcome) => [ - markdownCell(outcome.key), - markdownCell(outcome.status), - outcome.detail.map(markdownCell).join("
"), - ]), + input.outcomes.map((outcome, row) => { + const shown = outcome.detail.slice(0, kept.detail[row]).map(markdownCell); + const cut = outcome.detail.length - shown.length; + return [ + markdownCell(outcome.key), + markdownCell(outcome.status), + (cut > 0 ? [...shown, leftOutMark(cut)] : shown).join(BREAK), + ]; + }), ), ); } @@ -78,10 +108,67 @@ export function composeReport(input: ReportInput): string { if (input.transcript.length === 0) { lines.push("No output was captured for this target."); } else { - const body = input.transcript.map(transcriptLine).join("\n"); - const fence = fenceFor(body); - lines.push(fence, body, fence); + const shown = input.transcript.slice(0, kept.transcript).map(transcriptLine); + if (shown.length > 0) { + const body = shown.join("\n"); + const fence = fenceFor(body); + lines.push(fence, body, fence); + } + if (shown.length < input.transcript.length) { + lines.push( + ...(shown.length > 0 ? [""] : []), + leftOutMark(input.transcript.length - shown.length), + ); + } } lines.push(""); return lines.join("\n"); } + +/** + * The lines that fit under `limit`, spent in the order a reader loses least: each section's detail first, one line per + * section in turn so a long section never starves a short one, then the transcript from its start, which repeats the + * detail. A list ends at its first line that does not fit. Every line is paid for against the document that carries + * none, whose marks are the longest a cut renders, so the result is over the limit only when that document is. + */ +function fit(input: ReportInput, limit: number): Kept { + const kept: Kept = { detail: input.outcomes.map(() => 0), transcript: 0 }; + // What a first transcript line adds beyond itself: the two fences, their line ends, and the blank line above the mark. + const fences = 2 * fenceFor(input.transcript.map(transcriptLine).join("\n")).length + 3; + let room = limit - render(input, kept, limit).length - fences; + const take = (cost: number): boolean => { + if (cost > room) { + return false; + } + room -= cost; + return true; + }; + const open = new Set(input.outcomes.keys()); + for (let depth = 0; open.size > 0; depth++) { + for (const row of open) { + const line = input.outcomes[row]?.detail[depth]; + if (line !== undefined && take(markdownCell(line).length + BREAK.length)) { + kept.detail[row] = depth + 1; + } else { + open.delete(row); + } + } + } + for (const entry of input.transcript) { + if (!take(transcriptLine(entry).length + 1)) { + break; + } + kept.transcript += 1; + } + return kept; +} + +/** `limit` is the most characters the document may hold: over it, lines are left out and the document counts them. */ +export function composeReport(input: ReportInput, limit = Number.POSITIVE_INFINITY): string { + const all: Kept = { + detail: input.outcomes.map((outcome) => outcome.detail.length), + transcript: input.transcript.length, + }; + const whole = render(input, all, limit); + return whole.length <= limit ? whole : render(input, fit(input, limit), limit); +} diff --git a/src/report/delivery.ts b/src/report/delivery.ts index 4a545495d..8a35a0197 100644 --- a/src/report/delivery.ts +++ b/src/report/delivery.ts @@ -23,10 +23,12 @@ import { type ArtifactUploader, deliverArtifactReport } from "./artifact-report. import { composeReport } from "./composer.js"; import { deliverIssueReport, + ISSUE_BODY_LIMIT, type IssueReportMode, injectMarkerLabel, type LandedWrites, MARKER_LABEL, + mayHoldMarkerLabel, } from "./issue-report.js"; /** The channels, in the words of the `private-report` input; the type below says what each delivers. */ @@ -90,9 +92,10 @@ export interface ReportRunMeta { } /** - * `on` is false when the channel is off or the target is not redacted. The notice is returned rather than emitted, so - * the caller can route it through the target's capturing sink. An injected document is a new document, so it earns - * its brand where every document does: through validateSettingsDoc, the brand's one mint. + * `on` is what the target's channel answered (ReportChannel.declaresMarker): false when no issue channel reports on + * the target, or when the target holds no marker label to keep. The notice is returned rather than emitted, so the + * caller can route it through the target's capturing sink. An injected document is a new document, so it earns its + * brand where every document does: through validateSettingsDoc, the brand's one mint. */ export function applyMarkerInjection( settings: ValidatedSettings, @@ -168,7 +171,16 @@ interface ReportTarget { * content) and never changes any target's result. */ export interface ReportChannel { + /** + * Whether the target's settings must declare the marker label (applyMarkerInjection), asked before the target's + * work. Only the issue channel has a label to keep, and it says no for a target it reads as holding none. + */ + declaresMarker(repo: RepoRef | null): Promise; + + /** Delivers one target's report once the target's work is done. */ deliver(target: ReportTarget): Promise; + + /** Ends the run's deliveries; a channel that gathers its reports sends them here. */ flush(): Promise; } @@ -201,19 +213,25 @@ export function openReportChannel( } } -/** The seal opens in full here: the readers are the target repository's own, or hold the artifact's decryption key. */ -function composeTargetReport(meta: ReportRunMeta, target: ReportTarget): string { +/** + * The seal opens in full here: the readers are the target repository's own, or hold the artifact's decryption key. + * `limit` is the most characters the channel's document may hold; the report is cut to it and says so. + */ +function composeTargetReport(meta: ReportRunMeta, target: ReportTarget, limit?: number): string { const { slug, outcomes, transcript } = revealPrivate(target.detail); - return composeReport({ - target: slug, - adminRepo: meta.adminRepo, - runUrl: meta.runUrl, - mode: meta.mode, - result: target.conclusion.result, - timestamp: meta.timestamp, - outcomes: outcomes.map((o) => ({ key: o.key, status: o.status, detail: o.detail })), - transcript, - }); + return composeReport( + { + target: slug, + adminRepo: meta.adminRepo, + runUrl: meta.runUrl, + mode: meta.mode, + result: target.conclusion.result, + timestamp: meta.timestamp, + outcomes: outcomes.map((o) => ({ key: o.key, status: o.status, detail: o.detail })), + transcript, + }, + limit, + ); } function issueChannel( @@ -223,6 +241,8 @@ function issueChannel( io: Io, ): ReportChannel { return { + // An unparseable slug names no repository to read, and no settings are applied to it either. + declaresMarker: async (repo) => repo !== null && (await mayHoldMarkerLabel(api, repo)), async deliver(target) { if (target.repo === null) { // The issue channel posts INTO the target repository, and an unparseable slug names none; the loss must not be silent. @@ -232,7 +252,7 @@ function issueChannel( ); return; } - const body = composeTargetReport(meta, target); + const body = composeTargetReport(meta, target, ISSUE_BODY_LIMIT); const delivery = await deliverIssueReport( api, target.repo, @@ -279,6 +299,7 @@ function artifactChannel( ): ReportChannel { const reports: string[] = []; return { + declaresMarker: async () => false, async deliver(target) { reports.push(`\n\n${composeTargetReport(meta, target)}`); }, diff --git a/src/report/issue-report.ts b/src/report/issue-report.ts index c605dde7b..e67df6dc1 100644 --- a/src/report/issue-report.ts +++ b/src/report/issue-report.ts @@ -56,8 +56,19 @@ export const ISSUE_REPORT_ENDPOINTS = { route: "POST /repos/{owner}/{repo}/labels", statuses: { 201: "marker label created", 422: "the marker label already exists" }, }, + readLabel: { + route: "GET /repos/{owner}/{repo}/labels/{name}", + statuses: { 200: "the target holds the marker label", 404: "the target holds no marker label" }, + }, } as const satisfies Record; +/** + * The most characters GitHub keeps in an issue body. Its REST reference states no limit; the number is the one in the + * 422 a longer body earns on every attempt: "body is too long (maximum is 65536 characters)". A string's length counts + * UTF-16 units, never fewer than GitHub's characters, so a body within this length always fits. + */ +export const ISSUE_BODY_LIMIT = 65536; + export type IssueReportMode = "always" | "on-failure"; /** @@ -433,6 +444,25 @@ export async function deliverIssueReport( } } +/** + * False only when the target answers 404 for the marker label, asked before the target's settings are applied. + * Declaring the marker on such a target would make the labels section create it, and a check call a repository no + * report ever touched drifted. Every other answer may hide a label, a failed read included, and declares it, so an + * unread label is never deleted. Nothing is printed: the delivery that follows reports a failure that persists. + */ +export async function mayHoldMarkerLabel(api: GitHubClient, repo: RepoRef): Promise { + try { + const label = await api.tryRequest( + "GET", + expand(ISSUE_REPORT_ENDPOINTS.readLabel, { repo }, { name: MARKER_LABEL }), + ); + return !("error" in label && label.error.status === 404); + } catch { + // A client that throws breaks the GitHubClient contract; its message embeds the request path (the private slug), so nothing from it may escape. + return true; + } +} + /** * A declared `labels` section under the delete policy would DELETE the marker label an earlier run's report delivery * created, so the marker joins the declared set unless an entry already manages it. An entry renaming the marker AWAY diff --git a/test/e2e/fuzz.ts b/test/e2e/fuzz.ts index 538cc3cb4..74b6a0330 100644 --- a/test/e2e/fuzz.ts +++ b/test/e2e/fuzz.ts @@ -738,10 +738,14 @@ interface Counterfactual { artifactDir?: string; } -/** What a raw target may still receive, by first path segment: the parse gate stops it before any section call, and the report channel's marker-label ensure-create is POST-only, so a labels GET or PATCH is a section call leaking through. */ +/** + * What a raw target may still receive, by first path segment: the parse gate stops it before any section call. The + * report channel reads the marker label (GET) ahead of the parse and ensure-creates it (POST), so a labels PATCH or + * DELETE is a section call leaking through. + */ const RAW_TARGET_ALLOWED_BY_HEAD: Record> = { contents: new Set(["GET"]), - labels: new Set(["POST"]), + labels: new Set(["GET", "POST"]), issues: new Set(["GET", "POST", "PATCH"]), }; diff --git a/test/e2e/mock/chaos.ts b/test/e2e/mock/chaos.ts index bcfc6a5bd..5e0d7d2c6 100644 --- a/test/e2e/mock/chaos.ts +++ b/test/e2e/mock/chaos.ts @@ -16,6 +16,7 @@ import type { Json, MockResponse } from "./support.js"; const CORE_FAULT_KEYS = { "core.discoveryList": "GET /user/repos (multi-repo discovery listing)", "core.contentsGet": "GET /repos/{owner}/{repo}/contents/{path} (settings-file fetch)", + "core.reportLabelRead": `${ISSUE_REPORT_ENDPOINTS.readLabel.route} (report marker-label read)`, "core.reportLabelCreate": `${ISSUE_REPORT_ENDPOINTS.createLabel.route} (report marker-label ensure-create)`, "core.issuesList": `${ISSUE_REPORT_ENDPOINTS.list.route} (report issue lookup)`, "core.issueCreate": `${ISSUE_REPORT_ENDPOINTS.create.route} (report issue create)`, diff --git a/test/e2e/mock/core-paths.ts b/test/e2e/mock/core-paths.ts index 4a1f43704..c00bff38c 100644 --- a/test/e2e/mock/core-paths.ts +++ b/test/e2e/mock/core-paths.ts @@ -419,6 +419,39 @@ function serveMarkerLabelCreate( return { response: { status: 201, body: label }, coreKey }; } +/** + * The marker-label read precedes the target's work in both modes, so it is served here, beside the ensure-create, and + * only for the marker name on a delivery target; any other label GET falls through to section matching, where no + * route answers it. + */ +function serveMarkerLabelRead( + request: IssueReportRequest, + run: IssueReportRun, +): IssueReportOutcome | null { + const labelMatch = request.pathname.match(/^\/repos\/([^/]+\/[^/]+)\/labels\/([^/]+)$/); + if ( + !labelMatch || + request.method !== "GET" || + decodeURIComponent(labelMatch[2] ?? "") !== MARKER_LABEL + ) { + return null; + } + const slug = decodeURIComponent(labelMatch[1] ?? ""); + if (!isReportDeliveryTarget(slug, run.scenario, run.multi, run.faults)) { + return null; + } + const coreKey = "core.reportLabelRead" as const; + const admitted = admitIssueRoute(request, run, slug, coreKey); + if (!("state" in admitted)) { + return admitted; + } + const label = findLabel(admitted.state, MARKER_LABEL); + return { + response: label ? ok(label) : { status: 404, body: { message: "Not Found" } }, + coreKey, + }; +} + /** GitHub's default is newest first; the number stands in for created_at, which the seeded issues do not carry. */ function listIssues(state: MockState, query: Record): IssueReportOutcome { if (query.sort !== undefined && query.sort !== "created") { @@ -512,6 +545,7 @@ function serveIssueRoute( /** * Null when the path is not an issue-channel route, so the caller falls through to section matching. + * GET /repos/{o}/{r}/labels/{marker} -> the marker read (Issues: read) * POST /repos/{o}/{r}/labels (marker) -> ensure-create (Issues: write) * .../issues -> the issue routes */ @@ -519,7 +553,11 @@ export function handleIssueReport( request: IssueReportRequest, run: IssueReportRun, ): IssueReportOutcome | null { - return serveMarkerLabelCreate(request, run) ?? serveIssueRoute(request, run); + return ( + serveMarkerLabelRead(request, run) ?? + serveMarkerLabelCreate(request, run) ?? + serveIssueRoute(request, run) + ); } /** Wire attempts the probe can make, from the client's own MAX_RETRIES; once a slug's repository.get has faulted this many times the probe has given up, so the next repository.get is not a probe retry and the exemption expires. */ diff --git a/test/e2e/scenarios/multi-report-issue-on-failure-healthy-quiet.yml b/test/e2e/scenarios/multi-report-issue-on-failure-healthy-quiet.yml index 2cbede743..3ba8f24fd 100644 --- a/test/e2e/scenarios/multi-report-issue-on-failure-healthy-quiet.yml +++ b/test/e2e/scenarios/multi-report-issue-on-failure-healthy-quiet.yml @@ -1,9 +1,10 @@ # The issue-on-failure channel on a HEALTHY run: total quiet. svc-quiet is -# private and clean (the declared labels, marker included, match live), so the -# channel performs exactly one read - the open-issue lookup by the marker -# label - finds no open report issue, and writes NOTHING: no marker-label -# ensure-create, no issue create or update, no fallback title scan. A healthy -# repo never sees a report issue at all. +# private, converged, and has never needed a report, so neither its settings +# file nor the repository holds the marker label. The channel makes two reads: +# the marker label (404, so the label is not declared and the check stays +# clean) and the open-issue lookup by the marker label, which finds none. It +# writes NOTHING: no marker-label ensure-create, no issue create or update, no +# fallback title scan. A healthy repo never sees a report issue or its label. name: multi-report-issue-on-failure-healthy-quiet settings: {} inputs: @@ -15,9 +16,6 @@ repos: labels: - name: declared color: "00ff00" - - name: settings-as-code-report - color: "0e2a47" - description: "managed by settings-as-code private reporting - do not remove" live_state: repo: private: true @@ -25,9 +23,6 @@ repos: labels: - name: declared color: "00ff00" - - name: settings-as-code-report - color: "0e2a47" - description: "managed by settings-as-code private reporting - do not remove" permissions: issues: write contents: read @@ -41,8 +36,8 @@ expect: - "svc-quiet" stdout_lacks: - "svc-quiet" - # Zero report issues created; the one request the channel made was the - # marker-label lookup. + # Zero report issues created; the channel looked for an open one by the + # marker label. issue_report: slug: e2e-owner/svc-quiet created_count: 0 @@ -56,6 +51,8 @@ expect: # Both create-guard reads (the state=all label lookup and the title scan) share # the open lookup's path; their state=all query is what sets them apart. - "GET /repos/e2e-owner/svc-quiet/issues?state=all" - # The single read: an indexed lookup for an OPEN report issue by the marker. + # The two reads: the marker label, then an indexed lookup for an OPEN report + # issue by the marker. requests_contain: + - "GET /repos/e2e-owner/svc-quiet/labels/settings-as-code-report" - "GET /repos/e2e-owner/svc-quiet/issues?state=open&labels=settings-as-code-report&per_page=100" diff --git a/test/flows/deliver.test.ts b/test/flows/deliver.test.ts index 052410309..b4ea42981 100644 --- a/test/flows/deliver.test.ts +++ b/test/flows/deliver.test.ts @@ -145,35 +145,50 @@ describe("engineOutcome", () => { }); describe("withDelivery", () => { - test.each<[PrivateReportChannel, Exposure, boolean]>([ - ["none", PRIVATE, false], - ["issue", PRIVATE, true], - ["issue", { kind: "redacted", visibility: "internal" }, true], - ["issue", { kind: "redacted", visibility: "unknown" }, false], - ["issue", SHOWN, false], - ["issue-on-failure", PRIVATE, true], - ["artifact", PRIVATE, false], - ])("under %s a %j target injects the marker: %p", async (channel, exposure, injects) => { - // The artifact channel opens only with an upload port; nothing is delivered here. - const uploader: ArtifactUploader | undefined = - channel === "artifact" ? { upload: async () => ({ uploaded: true as const }) } : undefined; - await delivered( - cfg(channel, "apply"), - {}, - async (delivery, io) => { - const opened = { - repo: repo("o/priv"), - channel: publicChannel(io, "o/priv", true), - exposure, - }; - await delivery.target(opened, async (injectsMarker) => { - expect(injectsMarker).toBe(injects); - return outcome("clean"); - }); - }, - uploader, - ); - }); + // Who is asked: only an issue channel reads the marker label, and only for a target it will report on. + const markerRead = `GET /repos/o/priv/labels/${MARKER_LABEL}`; + // An unrouted GET is the double's 404: the target holds no marker label, so there is none to declare. + const LABEL = { held: { [markerRead]: { data: { name: MARKER_LABEL } } }, absent: {} }; + const INTERNAL: Exposure = { kind: "redacted", visibility: "internal" }; + const UNKNOWN: Exposure = { kind: "redacted", visibility: "unknown" }; + test.each<[PrivateReportChannel, Exposure, keyof typeof LABEL, boolean, number]>([ + ["none", PRIVATE, "held", false, 0], + ["issue", PRIVATE, "held", true, 1], + ["issue", INTERNAL, "held", true, 1], + ["issue", PRIVATE, "absent", false, 1], + ["issue", UNKNOWN, "held", false, 0], + ["issue", SHOWN, "held", false, 0], + ["issue-on-failure", PRIVATE, "held", true, 1], + ["issue-on-failure", PRIVATE, "absent", false, 1], + ["artifact", PRIVATE, "held", false, 0], + ])( + "under %s a %j target with the marker label %s injects it: %p, after %i read(s)", + async (channel, exposure, label, injects, reads) => { + // The artifact channel opens only with an upload port; nothing is delivered here. + const uploader: ArtifactUploader | undefined = + channel === "artifact" ? { upload: async () => ({ uploaded: true as const }) } : undefined; + const asked: string[] = []; + await delivered( + cfg(channel, "apply"), + LABEL[label], + async (delivery, io, events) => { + const opened = { + repo: repo("o/priv"), + channel: publicChannel(io, "o/priv", true), + exposure, + }; + await delivery.target(opened, async (injectsMarker) => { + // The read is over before the target's work starts, so the work plans against its answer. + asked.push(...events); + expect(injectsMarker).toBe(injects); + return outcome("clean"); + }); + }, + uploader, + ); + expect(asked).toEqual(Array(reads).fill(`api ${markerRead}`)); + }, + ); test("channel none: a redacted target closes sealed and speaks one closed-value line; a shown target closes open and stays silent", async () => { const { api } = await delivered(cfg("none", "apply"), {}, async (delivery, io, events) => { @@ -218,6 +233,7 @@ describe("withDelivery", () => { async () => outcome("drift", [{ key: "labels", status: "drift", detail: ["+ CANARY"] }]), ); expect(events).toEqual([ + `api GET /repos/o/priv/labels/${MARKER_LABEL}`, "api POST /repos/o/priv/labels", `api GET /repos/o/priv/issues?state=all&labels=${MARKER_LABEL}&per_page=100&page=1`, "api PATCH /repos/o/priv/issues/7", diff --git a/test/flows/multi.test.ts b/test/flows/multi.test.ts index 666f16bb4..b906b8006 100644 --- a/test/flows/multi.test.ts +++ b/test/flows/multi.test.ts @@ -6,6 +6,7 @@ import { DEFAULT_DISCOVERY_FILTERS } from "../../src/discovery/discover.js"; import { SectionSelection } from "../../src/engine/section-selection.js"; import { runMulti } from "../../src/flows/multi.js"; import type { TargetOutcome } from "../../src/flows/redact.js"; +import { DEFAULT_API_VERSION, GitHubApi } from "../../src/github/api.js"; import { isPrivate } from "../../src/private.js"; import { describeProblem, type Problem } from "../../src/problem.js"; import { @@ -14,9 +15,12 @@ import { type ArtifactUploader, } from "../../src/report/artifact-report.js"; import { REPORT_HEADING } from "../../src/report/composer.js"; -import { ISSUE_TITLE, MARKER_LABEL } from "../../src/report/issue-report.js"; +import { ISSUE_TITLE, MARKER_LABEL, MARKER_LABEL_CONFIG } from "../../src/report/issue-report.js"; +import { ADMIN_OWNER, ADMIN_SLUG } from "../e2e/constants.js"; +import { startMockServer } from "../e2e/mock/server.js"; +import { scenario } from "../e2e/mock/server-test-support.js"; import { captureIo } from "../io/capture.js"; -import { MockApi } from "../mock-api.js"; +import { MockApi, type Route } from "../mock-api.js"; import { withTempDir } from "../temp-dir.js"; /** True when the target closed sealed: the redaction decision, read from the brand. */ @@ -570,6 +574,21 @@ describe("runMulti redaction (private-repos: redact)", () => { })); }); +/** Answers an issue write the way GitHub does: 422 to a body over its 65536-character limit, on every attempt. */ +class BodyLimitApi extends MockApi { + override async tryRequest( + ...args: Parameters + ): ReturnType { + const [method, path, payload] = args; + const body = (payload as { body?: unknown } | undefined)?.body; + if (method !== "GET" && path.includes("/issues") && String(body ?? "").length > 65536) { + this.calls.push({ method, path, payload }); + return { error: { status: 422, message: "Validation Failed", body: "" } }; + } + return super.tryRequest(...args); + } +} + describe("runMulti private-report: issue wiring", () => { const listPath = (slug: string) => `GET /repos/${slug}/issues?state=all&labels=${MARKER_LABEL}&per_page=100&page=1`; @@ -584,7 +603,7 @@ describe("runMulti private-report: issue wiring", () => { { data?: unknown; error?: { status: number; message: string; body: string } } > = {}, ) { - return new MockApi({ + return new BodyLimitApi({ "GET /repos/o/priv": { data: { description: "CANARY-live", private: true } }, "GET /repos/o/priv/contents/.github/settings.yml": { data: 'repository:\n description: "CANARY-want"\n', @@ -636,13 +655,14 @@ describe("runMulti private-report: issue wiring", () => { expect(publicText).not.toContain("o/priv"); }); - test("marker label is injected when a labels section is declared, notice lands in the report only", async () => { + test("the marker label of a target that holds it is declared, and the notice lands in the report only", async () => { const api = reportApi({ "GET /repos/o/priv/contents/.github/settings.yml": { data: 'labels:\n - name: bug\n color: "d73a4a"\n', }, - "GET /repos/o/priv/labels?per_page=100&page=1": { data: [] }, - }).allowMutations("POST /repos/o/priv/labels", "PATCH /repos/o/priv/labels/bug"); + [`GET /repos/o/priv/labels/${MARKER_LABEL}`]: { data: MARKER_LABEL_CONFIG }, + "GET /repos/o/priv/labels?per_page=100&page=1": { data: [MARKER_LABEL_CONFIG] }, + }).allowMutations("POST /repos/o/priv/labels"); const { io, annotations } = captureIo(); await runMulti( api, @@ -663,6 +683,48 @@ describe("runMulti private-report: issue wiring", () => { expect(annotations.some((a) => a.includes(MARKER_LABEL))).toBe(false); }); + test("a report over GitHub's issue-body limit is delivered cut to fit, every drift line still in its section", async () => { + // Uncut, the body earns GitHub's 422 on every attempt: the report is never delivered and re-running never helps. + const live = Array.from({ length: 300 }, (_, i) => ({ + name: `area/component-${String(i).padStart(3, "0")}`, + color: "ededed", + description: null, + })); + const page = (n: number) => `GET /repos/o/priv/labels?per_page=100&page=${n}`; + const api = reportApi({ + "GET /repos/o/priv/contents/.github/settings.yml": { data: "labels: []\n" }, + [page(1)]: { data: live.slice(0, 100) }, + [page(2)]: { data: live.slice(100, 200) }, + [page(3)]: { data: live.slice(200) }, + [page(4)]: { data: [] }, + }); + const { io, annotations, logs } = captureIo(); + await runTargets( + api, + cfg({ + reposInput: "o/priv", + mode: "check", + privateRepos: "redact", + privateReport: "issue", + selfSlug: "admin/repo", + }), + io, + ); + expect(annotations.filter((a) => a.includes("could not deliver"))).toEqual([]); + expect(logs).toEqual(["report: updated issue #7 in private repository #1"]); + const patch = api.calls.find( + (c) => c.method === "PATCH" && c.path === "/repos/o/priv/issues/7", + ); + const body = String(((patch?.payload ?? {}) as { body?: unknown }).body ?? ""); + expect(body).toMatch( + /^\*\*Cut to fit 65536 characters\.\*\* Lines left out: \d+ of 600; each cut is marked where it was made\.$/m, + ); + // The transcript repeats the Sections table, so the cut falls there and no label is lost from the report. + for (const label of live) { + expect(body).toContain(`labels[${label.name}]: undeclared`); + } + }); + test("delivery failure warns safely and never changes the target result", async () => { const api = reportApi({ "PATCH /repos/o/priv/issues/7": { @@ -909,13 +971,61 @@ describe("runMulti private-report: issue-on-failure wiring", () => { expect(String(payload.body ?? "")).toContain("o/priv"); }); - test("marker injection still fires; its notice lands in the closing report only", async () => { + // The settings file declares one label and the repository holds it, so the target is converged whatever the channel. + const markerRead = `GET /repos/o/priv/labels/${MARKER_LABEL}`; + const bug = { name: "bug", color: "d73a4a", description: null }; + const absent: Route = { error: { status: 404, message: "Not Found", body: "" } }; + const unread: Route = { error: { status: 500, message: "Server Error", body: "" } }; + test.each<[string, "check" | "apply", Route, unknown[]]>([ + ["absent, in check", "check", absent, [bug]], + ["absent, in apply", "apply", absent, [bug]], + ["present, in check", "check", { data: MARKER_LABEL_CONFIG }, [bug, MARKER_LABEL_CONFIG]], + ["present, in apply", "apply", { data: MARKER_LABEL_CONFIG }, [bug, MARKER_LABEL_CONFIG]], + ["unread (HTTP 500), in apply", "apply", unread, [bug, MARKER_LABEL_CONFIG]], + ])( + "the marker label %s: a converged target plans nothing and nothing is written", + async (_name, mode, read, live) => { + // Declared for a target without the label, a check calls a repository no report ever touched drifted and an apply creates the label there. + // Left undeclared on a target that holds it, the label an earlier report created is drift in check and a DELETE in apply. + const api = new MockApi({ + "GET /repos/o/priv": { data: { private: true } }, + "GET /repos/o/priv/contents/.github/settings.yml": { + data: 'labels:\n - name: bug\n color: "d73a4a"\n', + }, + "GET /repos/o/priv/labels?per_page=100&page=1": { data: live }, + [markerRead]: read, + [openListPath("o/priv")]: { data: [] }, + }); + const { io, events } = captureIo(); + const targets = await runTargets( + api, + cfg({ + reposInput: "o/priv", + mode, + privateRepos: "redact", + privateReport: "issue-on-failure", + selfSlug: "admin/repo", + }), + io, + ); + // An apply with nothing to change still says "applied"; its proof of convergence is the empty write list. + expect(targets[0]?.result).toBe(mode === "check" ? "clean" : "applied"); + expect(api.mutations()).toEqual([]); + expect(events).toEqual([ + "mask: o/priv", + "log: report: nothing to deliver for private repository #1", + ]); + }, + ); + + test("the marker label of a target that holds it is declared; its notice lands in the closing report only", async () => { const api = new MockApi({ "GET /repos/o/priv": { data: { private: true } }, "GET /repos/o/priv/contents/.github/settings.yml": { data: 'labels:\n - name: bug\n color: "d73a4a"\n', }, - "GET /repos/o/priv/labels?per_page=100&page=1": { data: [] }, + [markerRead]: { data: MARKER_LABEL_CONFIG }, + "GET /repos/o/priv/labels?per_page=100&page=1": { data: [MARKER_LABEL_CONFIG] }, [openListPath("o/priv")]: { data: [issue7] }, "PATCH /repos/o/priv/issues/7": { data: { number: 7 } }, }).allowMutations("POST /repos/o/priv/labels"); @@ -931,11 +1041,13 @@ describe("runMulti private-report: issue-on-failure wiring", () => { }), io, ); - const created = api - .mutations() - .filter((c) => c.method === "POST" && c.path === "/repos/o/priv/labels") - .map((c) => (c.payload as { name?: string }).name); - expect(created).toContain(MARKER_LABEL); + // The apply's one label write creates "bug"; the marker it declared is left as it is. + expect( + api + .mutations() + .filter((c) => c.path.startsWith("/repos/o/priv/labels")) + .map((c) => [c.method, (c.payload as { name?: string }).name]), + ).toEqual([["POST", "bug"]]); const patch = api.calls.find( (c) => c.method === "PATCH" && c.path === "/repos/o/priv/issues/7", ); @@ -945,6 +1057,62 @@ describe("runMulti private-report: issue-on-failure wiring", () => { }); }); +describe("runMulti private-report: the marker read of a hidden target", () => { + test("its trace line is redacted under debug, like the target's other requests", async () => { + // The read is the first request that names the target after the visibility probe; made before the slug is masked, its trace line would print the private slug. + const slug = `${ADMIN_OWNER}/svc-hidden`; + const declared = [{ name: "declared", color: "00ff00" }]; + const handle = await startMockServer( + scenario({ + inputs: { mode: "check", private_report: "issue-on-failure" }, + repos: { + [slug]: { + settings: { labels: declared }, + live_state: { repo: { private: true, visibility: "private" }, labels: declared }, + permissions: { issues: "write", contents: "read", administration: "read" }, + }, + }, + }), + ); + try { + const trace: string[] = []; + const io = { ...captureIo().io, debug: (line: string) => trace.push(line) }; + const api = new GitHubApi({ + token: "t", + io, + baseUrl: handle.url, + apiVersion: DEFAULT_API_VERSION, + }); + const targets = await runTargets( + api, + cfg({ + reposInput: slug, + adminOwner: ADMIN_OWNER, + mode: "check", + privateRepos: "redact", + privateReport: "issue-on-failure", + selfSlug: ADMIN_SLUG, + }), + io, + ); + expect(targets.map((t) => [t.display, t.result])).toEqual([ + ["private repository #1", "clean"], + ]); + expect(handle.violations).toEqual([]); + // The marker read is the run's one 404, so the one traced 404 is its line. + expect( + handle.requests.filter((r) => r.status === 404).map((r) => `${r.method} ${r.pathname}`), + ).toEqual([`GET /repos/${slug}/labels/${MARKER_LABEL}`]); + expect(trace.filter((line) => line.includes("-> 404"))).toEqual([ + expect.stringMatching(/^GET -> 404 \(\d+ms\)$/), + ]); + expect(trace.join("\n")).not.toContain("svc-hidden"); + } finally { + await handle.stop(); + } + }); +}); + describe("runMulti private-report: artifact wiring", () => { /** A capturing uploader plus the age keypair the ciphertext decrypts with. */ async function artifactHarness() { diff --git a/test/report/composer.test.ts b/test/report/composer.test.ts index 528148ca4..a05766c44 100644 --- a/test/report/composer.test.ts +++ b/test/report/composer.test.ts @@ -93,3 +93,120 @@ describe("composeReport", () => { expect(report).not.toContain("| Section |"); }); }); + +describe("composeReport under a length limit", () => { + const detail = (section: string, count: number): string[] => + Array.from( + { length: count }, + (_, i) => `${section}[entry-${i}]: undeclared, so apply deletes it`, + ); + /** A drifted section whose transcript repeats its detail, as a check run's does. */ + const drifted = (count: number): ReportInput => { + const lines = detail("labels", count); + return input({ + outcomes: [{ key: "labels", status: "drift", detail: lines }], + transcript: lines.map((line) => ({ line: `drift: ${line}` })), + }); + }; + /** What a cut report says about itself: the counts in its head notice, and the count each mark carries. */ + const leftOut = (report: string) => ({ + head: /^\*\*Cut to fit (\d+) characters\.\*\* Lines left out: (\d+) of (\d+); /m + .exec(report) + ?.slice(1) + .map(Number), + marks: [...report.matchAll(/\(lines left out: (\d+)\)/g)].map((match) => Number(match[1])), + }); + const transcriptOf = (report: string): string => report.slice(report.indexOf("## Transcript")); + + test("a report within the limit is the whole report, with no notice", () => { + const report = drifted(5); + const whole = composeReport(report); + expect(composeReport(report, whole.length)).toBe(whole); + expect(leftOut(whole)).toEqual({ head: undefined, marks: [] }); + }); + + test("one character over, the transcript is cut and every detail line stays; the head and the mark count the cut", () => { + // Uncut, a body over the channel's limit is refused on every run, so the report is never delivered at all. + const report = drifted(40); + const whole = composeReport(report); + const limit = whole.length - 1; + const cut = composeReport(report, limit); + expect(cut.length).toBeLessThanOrEqual(limit); + const row = (text: string) => text.split("\n").find((line) => line.startsWith("| labels |")); + expect(row(cut)).toBe(row(whole) as string); + const { head, marks } = leftOut(cut); + expect(marks).toHaveLength(1); + expect(head).toEqual([limit, marks[0] as number, 80]); + // The transcript keeps its first lines, in order, under a closed fence; the mark stands below it. + const kept = 40 - (marks[0] as number); + expect(kept).toBeGreaterThan(0); + expect(transcriptOf(cut)).toBe( + [ + "## Transcript", + "", + "```", + ...(report.transcript.slice(0, kept).map((entry) => entry.line) as string[]), + "```", + "", + `(lines left out: ${marks[0]})`, + "", + ].join("\n"), + ); + }); + + test("when the detail alone is over the limit, sections take turns: a short section keeps its lines and the long one is cut", () => { + // Spent section by section, a long first section would take the whole limit and a failed section's one line would be the one left out. + const long = detail("labels", 60); + const report = input({ + outcomes: [ + { key: "labels", status: "drift", detail: long }, + { key: "rulesets", status: "failed", detail: ["rulesets: 403 Forbidden"] }, + ], + transcript: long.map((line) => ({ line })), + }); + const limit = Math.floor(composeReport(report).length / 3); + const cut = composeReport(report, limit); + expect(cut.length).toBeLessThanOrEqual(limit); + expect(cut).toContain("| rulesets | failed | rulesets: 403 Forbidden |"); + const { head, marks } = leftOut(cut); + // Two marks: the labels cell's, then the transcript's, which is left out whole and so has no fence. + expect(marks).toHaveLength(2); + expect(marks[1]).toBe(60); + expect(head).toEqual([limit, (marks[0] as number) + 60, 121]); + expect(cut).toContain(`${long[0]}
`); + expect(cut).toContain(`
(lines left out: ${marks[0]}) |`); + expect(transcriptOf(cut)).toBe("## Transcript\n\n(lines left out: 60)\n"); + }); + + test("every limit from the emptiest document up is met, and the head count is the sum of the marks", () => { + // The fit is paid for line by line against the emptiest document; one unpaid character anywhere is a refused body at some limit. + const varied = (prefix: string): string[] => + Array.from({ length: 12 }, (_, i) => `${prefix}${"x".repeat((i * 7) % 23)}`); + const report = input({ + outcomes: [ + { key: "labels", status: "drift", detail: ["a | b", "``` c", "", ...varied("d")] }, + { key: "repository", status: "clean", detail: [] }, + { key: "rulesets", status: "failed", detail: ["e\nf", ...varied("g")] }, + ], + transcript: [ + { line: "```" }, + { level: "warning", line: "" }, + ...varied("h").map((line) => ({ line })), + ], + }); + const whole = composeReport(report); + const lefts = new Set(); + const broken = Array.from({ length: whole.length }, (_, limit) => limit).filter((limit) => { + const cut = composeReport(report, limit); + const { head, marks } = leftOut(cut); + const left = marks.reduce((sum, count) => sum + count, 0); + lefts.add(left); + // Only the document that carries no line at all may be over the limit: nothing smaller exists. + return head?.join() !== [limit, left, 42].join() || (cut.length > limit && left < 42); + }); + expect(broken).toEqual([]); + // Not vacuous: the scan met cuts of more than twenty different sizes, the emptiest document included. + expect(lefts.size).toBeGreaterThan(20); + expect(lefts.has(42)).toBe(true); + }); +}); diff --git a/test/report/delivery.test.ts b/test/report/delivery.test.ts index 70a9dede7..ae423f31d 100644 --- a/test/report/delivery.test.ts +++ b/test/report/delivery.test.ts @@ -270,6 +270,29 @@ describe("the artifact channel", () => { expect(annotations).toEqual([]); }); + test("a report longer than an issue body holds is uploaded whole: only the issue channel cuts", async () => { + // The guide names this channel as the one that delivers the whole report; cut here too, the lines an issue leaves out would exist nowhere. + const { recipient, uploader, uploads, decrypt } = await harness(); + const detail = Array.from({ length: 700 }, (_, i) => `labels[label-${i}]: ${"x".repeat(90)}`); + const channel = openReportChannel( + new MockApi({}), + "artifact", + META, + recipient, + silentIo(), + uploader, + ); + await channel?.deliver({ + ...target("o/a", 1), + detail: sealed("o/a", [{ key: "labels", status: "drift", detail }]), + }); + await channel?.flush(); + const document = await decrypt(uploads[0]?.file.data as Uint8Array); + expect(document.length).toBeGreaterThan(65536); + expect(document).toContain(detail.at(-1) as string); + expect(document).not.toContain("Cut to fit"); + }); + test("flush uploads nothing when no target delivered", async () => { const { recipient, uploader, uploads } = await harness(); await openReportChannel( diff --git a/test/report/issue-report.test.ts b/test/report/issue-report.test.ts index d52396a1f..2a4718e9f 100644 --- a/test/report/issue-report.test.ts +++ b/test/report/issue-report.test.ts @@ -11,6 +11,7 @@ import { injectMarkerLabel, MARKER_LABEL, MARKER_LABEL_CONFIG, + mayHoldMarkerLabel, } from "../../src/report/issue-report.js"; import type { SettingsFile } from "../../src/schema.js"; import { MockApi, type Route } from "../mock-api.js"; @@ -506,6 +507,33 @@ describe("deliverIssueReport under mode: on-failure", () => { }); }); +describe("mayHoldMarkerLabel", () => { + const READ = `GET /repos/o/private-repo/labels/${MARKER_LABEL}`; + const failed = (status: number): Route => ({ error: { status, message: "CANARY", body: "" } }); + + // Only a 404 may leave the marker undeclared: were a failed read to do it too, an apply would delete a label it never saw. + test.each<[string, Route, boolean]>([ + ["the label (200)", { data: MARKER_LABEL_CONFIG }, true], + ["no such label (404)", failed(404), false], + ["a denial (403)", failed(403), true], + ["a server error (500)", failed(500), true], + ["no HTTP answer", { failed: `${READ}: the transport failed` }, true], + ])("%s answers %p, after one read", async (_name, route, held) => { + const api = new MockApi({ [READ]: route }); + expect(await mayHoldMarkerLabel(api, SLUG)).toBe(held); + expect(api.calls.map((c) => `${c.method} ${c.path}`)).toEqual([READ]); + }); + + test("a client that throws instead of answering is read like any failed read, and its message never escapes", async () => { + // The thrown message embeds the request path, which names the private slug. + const api = new MockApi({}); + api.tryRequest = async (method, path) => { + throw new Error(`${method} ${path}: connection reset`); + }; + expect(await mayHoldMarkerLabel(api, SLUG)).toBe(true); + }); +}); + describe("injectMarkerLabel", () => { type Outcome = ReturnType["outcome"]; const bug = { name: "bug", color: "d73a4a" }; From f8c47048fd4fb6ca3e519b71e6d6c59d7fd22885 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 22:34:05 -0500 Subject: [PATCH 2/2] fix(report): keep the report limit out of the library entry and prove the marker rule's remaining cells composeReport is again composeReport(input) for a library caller. The cut is composeReportWithin, which the library entry does not export and the issue channel calls with GitHub's limit, so no caller can ask for a limit below the report's fixed part. The cut's contract is stated as the code keeps it: detail and transcript lines never push the body past the limit, and the fixed part (the heading, the notice, the run table, each section's status, the marks) is never cut, so a limit below that part is not met. The composer test asserts both sides of that part instead of excusing one. The page states the marker rule as a table of the read's answer by mode, including what a failed read costs on a repository without the label (drift in check, a created label in apply), and no longer says that a healthy run only reads. The flow table covers all eight cells of that rule, and a new scenario applies and then checks a private target that holds the marker label without declaring it. --- docs/operate/private-repositories.md | 21 +++-- src/report/composer.ts | 26 ++++-- src/report/delivery.ts | 36 ++++---- src/report/issue-report.ts | 14 ++- test/e2e/fuzz.ts | 6 +- ...-report-issue-on-failure-healthy-quiet.yml | 10 +-- ...ti-report-issue-on-failure-marker-kept.yml | 68 ++++++++++++++ test/flows/multi.test.ts | 88 ++++++++++++++----- test/report/composer.test.ts | 48 ++++++---- 9 files changed, 221 insertions(+), 96 deletions(-) create mode 100644 test/e2e/scenarios/multi-report-issue-on-failure-marker-kept.yml diff --git a/docs/operate/private-repositories.md b/docs/operate/private-repositories.md index 8f1e5598a..e4df485e0 100644 --- a/docs/operate/private-repositories.md +++ b/docs/operate/private-repositories.md @@ -87,23 +87,32 @@ Every other channel follows the same four rules: - **Which issue:** found by the marker label, or by its exact title when someone removed the label. Either way it must be one of the action's own reports: the body carries the report heading. A same-titled issue you opened by hand is left alone, unless its body pastes a report verbatim. - **Whose issue:** the creator does not matter, so a rotated PAT reuses the issue instead of opening a second one. With several reports, an open one wins, then the newest. - **Each run:** the body is replaced; the issue opens when the target fails or drifts and closes when it is healthy. -- **A long report is cut to fit:** GitHub keeps at most 65,536 characters in an issue body. A longer report keeps every section's status and leaves lines out: from the transcript first, then from the sections with the most detail. It opens by saying how many lines were left out, and each cut is marked in place. The `artifact` channel delivers the whole report. -- **The marker label:** the first delivery creates `settings-as-code-report`. Each run reads it once per target, before that target's settings are checked or applied. On a repository that holds it, a declared `labels` section manages it like any declared label, so it is not deleted as undeclared. On a repository without it, nothing is added. A read that fails counts as held, so the label is never deleted unread. +- **A long report is cut to fit:** GitHub keeps at most 65,536 characters in an issue body. A longer report keeps every section's status and leaves lines out: from the transcript first, then from the sections with the most detail. It opens by saying how many lines were left out and marks each cut in place, while the `artifact` channel delivers the whole report. - **What the log says:** every write is announced with the placeholder, never the slug: `report: updated issue #7 in private repository #1` (or `created`), and `report: created label "settings-as-code-report" in private repository #1` when the delivery creates the marker label. A write that landed before a later step failed is announced the same way, ahead of the warning. - **Permission:** the PAT needs `"Issues"` (read and write) on every target repository, on top of the section permissions. +The first delivery creates the marker label, `settings-as-code-report`. Each run reads it once per target, before that target's settings are checked or applied. Where the settings declare `labels` without naming the marker, the answer decides what that section does with it: + +| The read answers | The marker is | Check | Apply | +| ------------------------------ | ------------- | ------------------------------------------------------ | ----------------------------------------------- | +| 404: no such label | left out | says nothing about it | does not create it | +| 200: the label exists | declared | compares it like any declared label | keeps it | +| anything else: the read failed | declared | reports `drift` where the repository has no such label | creates the label where the repository has none | + +A failed read counts as a label that exists, so no label is deleted unread. A settings file that names the marker itself is outside the table: its own entry manages the label, and a rename away from the marker's name is dropped unless the read answers 404. + `private-report: issue-on-failure` is the quiet variant of `issue`. When a target fails (or drifts in check mode) it behaves identically: the reused, marker-labelled report issue is written and opened. -On a healthy run it only reads: the marker label, then an open report issue carrying it: +On a healthy run it reads the marker label, then looks for an open report issue carrying it: - **One is open,** left over from an earlier failure: it is updated with the healthy report and closed. -- **None is open:** nothing is written, no issue, no label, zero notification noise, and the log says so: `report: nothing to deliver for private repository #1`. +- **None is open:** the delivery writes nothing, no issue, no label, zero notification noise, and the log says so: `report: nothing to deliver for private repository #1`. -A repository that never fails never sees an issue or the marker label: both appear only when a report is delivered. The cost is that healthy runs' reports are not delivered anywhere. Choose `issue` or `artifact` when you need the report mirror on every run. +A repository that never fails never sees an issue, and no delivery creates the marker label there. The cost is that healthy runs' reports are not delivered anywhere. Choose `issue` or `artifact` when you need the report mirror on every run. One caveat on the quiet variant: a removed marker label hides an open issue. If someone removes the label while a report issue is open, the healthy path cannot find the issue, so it stays open until the next failing run recreates the label and reclaims it. -Everything else matches `issue`: the same delivery gate, the same Issues grant (a healthy run only reads; the open and close are writes), and delivery still runs in `mode: check`. +Everything else matches `issue`: the same delivery gate, the same Issues grant (with no open issue, a healthy run's delivery only reads; the open and the close are writes), and delivery still runs in `mode: check`. `private-report: artifact` concatenates the report for every proven-private target into one document, encrypts it to an age recipient, and uploads it as the workflow artifact `settings-as-code-private-report` (file `private-report.md.age`). Use it when the people who need the report cannot be given repository access, since the archive travels with the run rather than living in the target repo. diff --git a/src/report/composer.ts b/src/report/composer.ts index e78d7c7f9..1cfa0d559 100644 --- a/src/report/composer.ts +++ b/src/report/composer.ts @@ -1,6 +1,6 @@ /** * One markdown document per redacted target, rendered identically for both channels, except that a channel whose - * document has a length limit gets it cut to fit. Pure, and structurally typed on purpose, so it depends on no + * document has a length limit gets its lines cut to fit. Pure, and structurally typed on purpose, so it depends on no * action-layer types. */ @@ -126,10 +126,10 @@ function render(input: ReportInput, kept: Kept, limit: number): string { } /** - * The lines that fit under `limit`, spent in the order a reader loses least: each section's detail first, one line per - * section in turn so a long section never starves a short one, then the transcript from its start, which repeats the - * detail. A list ends at its first line that does not fit. Every line is paid for against the document that carries - * none, whose marks are the longest a cut renders, so the result is over the limit only when that document is. + * The lines the document carries under `limit`: each section's detail first, one line per section in turn, then the + * transcript from its start, each list ending at its first line that does not fit. These lines never push the + * document past the limit. The rest (heading, run table, section statuses, marks) is never cut, so a limit below it + * is not met. */ function fit(input: ReportInput, limit: number): Kept { const kept: Kept = { detail: input.outcomes.map(() => 0), transcript: 0 }; @@ -163,12 +163,20 @@ function fit(input: ReportInput, limit: number): Kept { return kept; } -/** `limit` is the most characters the document may hold: over it, lines are left out and the document counts them. */ -export function composeReport(input: ReportInput, limit = Number.POSITIVE_INFINITY): string { - const all: Kept = { +function everyLine(input: ReportInput): Kept { + return { detail: input.outcomes.map((outcome) => outcome.detail.length), transcript: input.transcript.length, }; - const whole = render(input, all, limit); +} + +/** The whole report, whatever its length. */ +export function composeReport(input: ReportInput): string { + return render(input, everyLine(input), Number.POSITIVE_INFINITY); +} + +/** The report for a channel whose document holds `limit` characters: over it, lines are left out and the document counts them. */ +export function composeReportWithin(input: ReportInput, limit: number): string { + const whole = render(input, everyLine(input), limit); return whole.length <= limit ? whole : render(input, fit(input, limit), limit); } diff --git a/src/report/delivery.ts b/src/report/delivery.ts index 8a35a0197..8663836af 100644 --- a/src/report/delivery.ts +++ b/src/report/delivery.ts @@ -20,7 +20,7 @@ import { revealPrivate } from "../private-open.js"; import { describeProblem } from "../problem.js"; import type { SectionKey, SettingsFile } from "../schema.js"; import { type ArtifactUploader, deliverArtifactReport } from "./artifact-report.js"; -import { composeReport } from "./composer.js"; +import { composeReport, composeReportWithin, type ReportInput } from "./composer.js"; import { deliverIssueReport, ISSUE_BODY_LIMIT, @@ -213,25 +213,19 @@ export function openReportChannel( } } -/** - * The seal opens in full here: the readers are the target repository's own, or hold the artifact's decryption key. - * `limit` is the most characters the channel's document may hold; the report is cut to it and says so. - */ -function composeTargetReport(meta: ReportRunMeta, target: ReportTarget, limit?: number): string { +/** The seal opens in full here: the readers are the target repository's own, or hold the artifact's decryption key. */ +function reportOf(meta: ReportRunMeta, target: ReportTarget): ReportInput { const { slug, outcomes, transcript } = revealPrivate(target.detail); - return composeReport( - { - target: slug, - adminRepo: meta.adminRepo, - runUrl: meta.runUrl, - mode: meta.mode, - result: target.conclusion.result, - timestamp: meta.timestamp, - outcomes: outcomes.map((o) => ({ key: o.key, status: o.status, detail: o.detail })), - transcript, - }, - limit, - ); + return { + target: slug, + adminRepo: meta.adminRepo, + runUrl: meta.runUrl, + mode: meta.mode, + result: target.conclusion.result, + timestamp: meta.timestamp, + outcomes: outcomes.map((o) => ({ key: o.key, status: o.status, detail: o.detail })), + transcript, + }; } function issueChannel( @@ -252,7 +246,7 @@ function issueChannel( ); return; } - const body = composeTargetReport(meta, target, ISSUE_BODY_LIMIT); + const body = composeReportWithin(reportOf(meta, target), ISSUE_BODY_LIMIT); const delivery = await deliverIssueReport( api, target.repo, @@ -301,7 +295,7 @@ function artifactChannel( return { declaresMarker: async () => false, async deliver(target) { - reports.push(`\n\n${composeTargetReport(meta, target)}`); + reports.push(`\n\n${composeReport(reportOf(meta, target))}`); }, async flush() { if (reports.length === 0) { diff --git a/src/report/issue-report.ts b/src/report/issue-report.ts index e67df6dc1..d183b9554 100644 --- a/src/report/issue-report.ts +++ b/src/report/issue-report.ts @@ -62,11 +62,7 @@ export const ISSUE_REPORT_ENDPOINTS = { }, } as const satisfies Record; -/** - * The most characters GitHub keeps in an issue body. Its REST reference states no limit; the number is the one in the - * 422 a longer body earns on every attempt: "body is too long (maximum is 65536 characters)". A string's length counts - * UTF-16 units, never fewer than GitHub's characters, so a body within this length always fits. - */ +/** GitHub's cap on an issue body, from the 422 a longer one earns every time ("body is too long (maximum is 65536 characters)"); its REST reference states none. A string's length in UTF-16 units is never below that count. */ export const ISSUE_BODY_LIMIT = 65536; export type IssueReportMode = "always" | "on-failure"; @@ -445,10 +441,10 @@ export async function deliverIssueReport( } /** - * False only when the target answers 404 for the marker label, asked before the target's settings are applied. - * Declaring the marker on such a target would make the labels section create it, and a check call a repository no - * report ever touched drifted. Every other answer may hide a label, a failed read included, and declares it, so an - * unread label is never deleted. Nothing is printed: the delivery that follows reports a failure that persists. + * False only when the target answers 404 for the marker label. Declared there, the label would be created by the + * labels section, and a check would call a repository no report ever touched drifted. Any other answer, a failed read + * included, declares it without printing anything, so no label is deleted unread and the delivery that follows + * reports a failure that persists. */ export async function mayHoldMarkerLabel(api: GitHubClient, repo: RepoRef): Promise { try { diff --git a/test/e2e/fuzz.ts b/test/e2e/fuzz.ts index 74b6a0330..25b83ab11 100644 --- a/test/e2e/fuzz.ts +++ b/test/e2e/fuzz.ts @@ -738,11 +738,7 @@ interface Counterfactual { artifactDir?: string; } -/** - * What a raw target may still receive, by first path segment: the parse gate stops it before any section call. The - * report channel reads the marker label (GET) ahead of the parse and ensure-creates it (POST), so a labels PATCH or - * DELETE is a section call leaking through. - */ +/** What a raw target may still receive, by first path segment: the parse gate stops it before any section call, and the report channel reads the marker label (GET) and ensure-creates it (POST), so a labels PATCH or DELETE is a leaked section call. */ const RAW_TARGET_ALLOWED_BY_HEAD: Record> = { contents: new Set(["GET"]), labels: new Set(["GET", "POST"]), diff --git a/test/e2e/scenarios/multi-report-issue-on-failure-healthy-quiet.yml b/test/e2e/scenarios/multi-report-issue-on-failure-healthy-quiet.yml index 3ba8f24fd..43af0789e 100644 --- a/test/e2e/scenarios/multi-report-issue-on-failure-healthy-quiet.yml +++ b/test/e2e/scenarios/multi-report-issue-on-failure-healthy-quiet.yml @@ -1,10 +1,8 @@ # The issue-on-failure channel on a HEALTHY run: total quiet. svc-quiet is -# private, converged, and has never needed a report, so neither its settings -# file nor the repository holds the marker label. The channel makes two reads: -# the marker label (404, so the label is not declared and the check stays -# clean) and the open-issue lookup by the marker label, which finds none. It -# writes NOTHING: no marker-label ensure-create, no issue create or update, no -# fallback title scan. A healthy repo never sees a report issue or its label. +# private and converged and never needed a report, so neither its settings nor +# the repository holds the marker label. The channel reads the marker label +# (404: not declared, the check stays clean) and looks up an open report +# issue, finds none, and writes NOTHING: no label, no issue, no title scan. name: multi-report-issue-on-failure-healthy-quiet settings: {} inputs: diff --git a/test/e2e/scenarios/multi-report-issue-on-failure-marker-kept.yml b/test/e2e/scenarios/multi-report-issue-on-failure-marker-kept.yml new file mode 100644 index 000000000..63f456dc6 --- /dev/null +++ b/test/e2e/scenarios/multi-report-issue-on-failure-marker-kept.yml @@ -0,0 +1,68 @@ +# The other side of the marker label: a repository that already holds it +# keeps it. An earlier report left svc-kept the label and a closed report +# issue, and its settings declare `labels` without the marker, where labels +# deletes what is undeclared. The marker read (200) declares the label for this +# run, so the apply writes no label and the check that follows exits 0 with +# zero writes. +name: multi-report-issue-on-failure-marker-kept +settings: {} +inputs: + mode: apply + private_report: issue-on-failure +repos: + e2e-owner/svc-kept: + settings: + labels: + - name: declared + color: "00ff00" + live_state: + repo: + private: true + visibility: private + labels: + - name: declared + color: "00ff00" + - name: settings-as-code-report + color: "0e2a47" + description: "managed by settings-as-code private reporting - do not remove" + issues: + - number: 7 + title: "[automated] settings-as-code: private settings report" + body: "# settings-as-code private report: e2e-owner/svc-kept\n\nOLD REPORT BODY" + state: closed + labels: + - name: settings-as-code-report + user: + login: e2e-token-user + html_url: "https://github.com/e2e-owner/svc-kept/issues/7" + permissions: + issues: write + contents: read + administration: read +expect: + exit_code: 0 + result: applied + repos_result: + "private repository #1": applied + summary_lacks: + - "svc-kept" + stdout_lacks: + - "svc-kept" + issue_report: + slug: e2e-owner/svc-kept + created_count: 0 + lookup_by_label: true + # No label is written in either direction, and the closed issue is left + # alone. + never: + - "DELETE /repos/e2e-owner/svc-kept/labels" + - "POST /repos/e2e-owner/svc-kept/labels" + - "PATCH /repos/e2e-owner/svc-kept/labels" + - "POST /repos/e2e-owner/svc-kept/issues" + - "PATCH /repos/e2e-owner/svc-kept/issues" + # The read that declares the marker for this run. + requests_contain: + - "GET /repos/e2e-owner/svc-kept/labels/settings-as-code-report" + # The check half: a rerun in check mode against the same repository must + # exit 0 with zero writes. + fixpoint: converges diff --git a/test/flows/multi.test.ts b/test/flows/multi.test.ts index b906b8006..d8699cd3d 100644 --- a/test/flows/multi.test.ts +++ b/test/flows/multi.test.ts @@ -5,7 +5,7 @@ import { Decrypter, generateX25519Identity, identityToRecipient } from "age-encr import { DEFAULT_DISCOVERY_FILTERS } from "../../src/discovery/discover.js"; import { SectionSelection } from "../../src/engine/section-selection.js"; import { runMulti } from "../../src/flows/multi.js"; -import type { TargetOutcome } from "../../src/flows/redact.js"; +import { REDACTED_NOTE, type TargetOutcome } from "../../src/flows/redact.js"; import { DEFAULT_API_VERSION, GitHubApi } from "../../src/github/api.js"; import { isPrivate } from "../../src/private.js"; import { describeProblem, type Problem } from "../../src/problem.js"; @@ -971,22 +971,62 @@ describe("runMulti private-report: issue-on-failure wiring", () => { expect(String(payload.body ?? "")).toContain("o/priv"); }); - // The settings file declares one label and the repository holds it, so the target is converged whatever the channel. + // The settings file declares one label, "bug", and the repository holds it; only the marker label varies. const markerRead = `GET /repos/o/priv/labels/${MARKER_LABEL}`; const bug = { name: "bug", color: "d73a4a", description: null }; - const absent: Route = { error: { status: 404, message: "Not Found", body: "" } }; const unread: Route = { error: { status: 500, message: "Server Error", body: "" } }; - test.each<[string, "check" | "apply", Route, unknown[]]>([ - ["absent, in check", "check", absent, [bug]], - ["absent, in apply", "apply", absent, [bug]], - ["present, in check", "check", { data: MARKER_LABEL_CONFIG }, [bug, MARKER_LABEL_CONFIG]], - ["present, in apply", "apply", { data: MARKER_LABEL_CONFIG }, [bug, MARKER_LABEL_CONFIG]], - ["unread (HTTP 500), in apply", "apply", unread, [bug, MARKER_LABEL_CONFIG]], + /** What the marker read answers, beside the labels the repository really holds. */ + const MARKER = { + absent: { read: { error: { status: 404, message: "Not Found", body: "" } }, live: [bug] }, + held: { read: { data: MARKER_LABEL_CONFIG }, live: [bug, MARKER_LABEL_CONFIG] }, + unreadHeld: { read: unread, live: [bug, MARKER_LABEL_CONFIG] }, + unreadAbsent: { read: unread, live: [bug] }, + } satisfies Record; + const QUIET = ["log: report: nothing to deliver for private repository #1"]; + const MARKER_POST = `POST /repos/o/priv/labels (${MARKER_LABEL})`; + /** A target that plans nothing: an apply with nothing to change still says "applied", and its proof is the empty write list. */ + const converged = (result: "clean" | "applied") => ({ result, writes: [], said: QUIET }); + test.each< + [ + string, + "check" | "apply", + keyof typeof MARKER, + { result: "clean" | "applied" | "drift"; writes: string[]; said: string[] }, + ] + >([ + ["404", "check", "absent", converged("clean")], + ["404", "apply", "absent", converged("applied")], + ["the label", "check", "held", converged("clean")], + ["the label", "apply", "held", converged("applied")], + ["500 over a label that exists", "check", "unreadHeld", converged("clean")], + ["500 over a label that exists", "apply", "unreadHeld", converged("applied")], + // The price of never deleting a label unread: a failed read declares the marker where the repository has none. + [ + "500 over no label", + "check", + "unreadAbsent", + { + result: "drift", + writes: [MARKER_POST, "POST /repos/o/priv/issues"], + said: [ + `log: report: created label "${MARKER_LABEL}" in private repository #1`, + "log: report: created issue #9 in private repository #1", + `annotate warning: private repository #1: drift - labels. ${REDACTED_NOTE}`, + ], + }, + ], + [ + "500 over no label", + "apply", + "unreadAbsent", + { result: "applied", writes: [MARKER_POST], said: QUIET }, + ], ])( - "the marker label %s: a converged target plans nothing and nothing is written", - async (_name, mode, read, live) => { - // Declared for a target without the label, a check calls a repository no report ever touched drifted and an apply creates the label there. - // Left undeclared on a target that holds it, the label an earlier report created is drift in check and a DELETE in apply. + "the marker read answers %s, in %s: the declared labels plan and write only what that answer allows", + async (_name, mode, marker, planned) => { + // Declared on a 404, a check calls a repository no report ever touched drifted and an apply creates the label there. + // Left undeclared where the label exists, an earlier report's label is drift in check and a DELETE in apply. + const { read, live } = MARKER[marker]; const api = new MockApi({ "GET /repos/o/priv": { data: { private: true } }, "GET /repos/o/priv/contents/.github/settings.yml": { @@ -995,7 +1035,12 @@ describe("runMulti private-report: issue-on-failure wiring", () => { "GET /repos/o/priv/labels?per_page=100&page=1": { data: live }, [markerRead]: read, [openListPath("o/priv")]: { data: [] }, - }); + [allListPath("o/priv")]: { data: [] }, + "GET /repos/o/priv/issues?state=all&sort=created&direction=desc&per_page=100&page=1": { + data: [], + }, + "POST /repos/o/priv/issues": { data: { number: 9 } }, + }).allowMutations("POST /repos/o/priv/labels", "DELETE /repos/o/priv/labels/*"); const { io, events } = captureIo(); const targets = await runTargets( api, @@ -1008,13 +1053,14 @@ describe("runMulti private-report: issue-on-failure wiring", () => { }), io, ); - // An apply with nothing to change still says "applied"; its proof of convergence is the empty write list. - expect(targets[0]?.result).toBe(mode === "check" ? "clean" : "applied"); - expect(api.mutations()).toEqual([]); - expect(events).toEqual([ - "mask: o/priv", - "log: report: nothing to deliver for private repository #1", - ]); + expect(targets[0]?.result).toBe(planned.result); + expect( + api.mutations().map((c) => { + const name = (c.payload as { name?: string } | undefined)?.name; + return `${c.method} ${c.path}${name === undefined ? "" : ` (${name})`}`; + }), + ).toEqual(planned.writes); + expect(events).toEqual(["mask: o/priv", ...planned.said]); }, ); diff --git a/test/report/composer.test.ts b/test/report/composer.test.ts index a05766c44..5b7e0b063 100644 --- a/test/report/composer.test.ts +++ b/test/report/composer.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { composeReport, type ReportInput } from "../../src/report/composer.js"; +import { composeReport, composeReportWithin, type ReportInput } from "../../src/report/composer.js"; function input(overrides: Partial = {}): ReportInput { return { @@ -94,7 +94,7 @@ describe("composeReport", () => { }); }); -describe("composeReport under a length limit", () => { +describe("composeReportWithin", () => { const detail = (section: string, count: number): string[] => Array.from( { length: count }, @@ -121,7 +121,7 @@ describe("composeReport under a length limit", () => { test("a report within the limit is the whole report, with no notice", () => { const report = drifted(5); const whole = composeReport(report); - expect(composeReport(report, whole.length)).toBe(whole); + expect(composeReportWithin(report, whole.length)).toBe(whole); expect(leftOut(whole)).toEqual({ head: undefined, marks: [] }); }); @@ -130,7 +130,7 @@ describe("composeReport under a length limit", () => { const report = drifted(40); const whole = composeReport(report); const limit = whole.length - 1; - const cut = composeReport(report, limit); + const cut = composeReportWithin(report, limit); expect(cut.length).toBeLessThanOrEqual(limit); const row = (text: string) => text.split("\n").find((line) => line.startsWith("| labels |")); expect(row(cut)).toBe(row(whole) as string); @@ -165,7 +165,7 @@ describe("composeReport under a length limit", () => { transcript: long.map((line) => ({ line })), }); const limit = Math.floor(composeReport(report).length / 3); - const cut = composeReport(report, limit); + const cut = composeReportWithin(report, limit); expect(cut.length).toBeLessThanOrEqual(limit); expect(cut).toContain("| rulesets | failed | rulesets: 403 Forbidden |"); const { head, marks } = leftOut(cut); @@ -178,8 +178,8 @@ describe("composeReport under a length limit", () => { expect(transcriptOf(cut)).toBe("## Transcript\n\n(lines left out: 60)\n"); }); - test("every limit from the emptiest document up is met, and the head count is the sum of the marks", () => { - // The fit is paid for line by line against the emptiest document; one unpaid character anywhere is a refused body at some limit. + test("detail and transcript lines never push the body past the limit; the fixed part is not cut, so a limit below it is not met", () => { + // One unpaid character in the fit is a refused body at some limit, so every limit up to the whole report is tried. const varied = (prefix: string): string[] => Array.from({ length: 12 }, (_, i) => `${prefix}${"x".repeat((i * 7) % 23)}`); const report = input({ @@ -194,19 +194,29 @@ describe("composeReport under a length limit", () => { ...varied("h").map((line) => ({ line })), ], }); - const whole = composeReport(report); - const lefts = new Set(); - const broken = Array.from({ length: whole.length }, (_, limit) => limit).filter((limit) => { - const cut = composeReport(report, limit); + // The fixed part is the body with all 42 lines left out: the heading, the notice, the run table, each section's + // status, and the marks. Its length moves only with the digits of the limit its notice names. + const fixedPart = (limit: number): number => + composeReportWithin(report, 0).length + String(limit).length - 1; + const cuts = Array.from({ length: composeReport(report).length }, (_, limit) => { + const cut = composeReportWithin(report, limit); const { head, marks } = leftOut(cut); - const left = marks.reduce((sum, count) => sum + count, 0); - lefts.add(left); - // Only the document that carries no line at all may be over the limit: nothing smaller exists. - return head?.join() !== [limit, left, 42].join() || (cut.length > limit && left < 42); + return { limit, length: cut.length, head, left: marks.reduce((sum, n) => sum + n, 0) }; }); - expect(broken).toEqual([]); - // Not vacuous: the scan met cuts of more than twenty different sizes, the emptiest document included. - expect(lefts.size).toBeGreaterThan(20); - expect(lefts.has(42)).toBe(true); + const limitsOf = (found: typeof cuts): number[] => found.map((cut) => cut.limit); + const reachable = cuts.filter((cut) => cut.limit >= fixedPart(cut.limit)); + const below = cuts.filter((cut) => cut.limit < fixedPart(cut.limit)); + + expect(limitsOf(reachable.filter((cut) => cut.length > cut.limit))).toEqual([]); + // Below the fixed part nothing more can go: the body is that part, every line left out, and over the limit. + expect( + limitsOf(below.filter((cut) => cut.length !== fixedPart(cut.limit) || cut.left !== 42)), + ).toEqual([]); + expect( + limitsOf(cuts.filter((cut) => cut.head?.join() !== [cut.limit, cut.left, 42].join())), + ).toEqual([]); + // Not vacuous: both sides of the fixed part were tried, and the reachable side met cuts of more than twenty sizes. + expect(below.length).toBeGreaterThan(500); + expect(new Set(reachable.map((cut) => cut.left)).size).toBeGreaterThan(20); }); });