diff --git a/README.md b/README.md index fbbe6562..12d525bf 100644 --- a/README.md +++ b/README.md @@ -304,8 +304,11 @@ answer where the next decision is actually being made. objections) as Markdown or `QueueReport` v1 JSON; also a managed-issue Action. - **CI comment** — the `@adrkit/ci` GitHub Action surfaces the governing decisions on the PRs that touch or explicitly declare them; pattern matches render as `via` - and PR-authored marker claims as `declared by`. It runs with only the default `GITHUB_TOKEN` and - degrades (never fails the job) on a read-only fork token. + and PR-authored marker claims as `declared by`. The comment separately reports + files it could not inspect for markers and markers it inspected but could not + resolve. Both reports are advisory, deterministically capped, and never influence + the Action's verdict. It runs with only the default `GITHUB_TOKEN` and degrades + (never fails the job) on a read-only fork token. - **MCP server** — let agents retrieve prior decisions, including the rejected ones, before proposing something already tried. @@ -323,6 +326,11 @@ steps: - uses: mbeacom/adrkit/packages/ci@v0 ``` +Keep the pull request head checkout at `GITHUB_WORKSPACE`. If `actions/checkout` +uses a different `ref` or a nested `path`, the Action can still resolve +corpus-authored `affects` matchers, but it cannot reliably inspect the changed +files for inbound `@adr` markers and says so in its comment. + ## Why not plain MADR — or "Structured MADR"? adrkit's frontmatter is a strict [MADR](https://adr.github.io/madr/) superset, so diff --git a/packages/ci/dist/index.js b/packages/ci/dist/index.js index 5b330e83..1cda4336 100644 --- a/packages/ci/dist/index.js +++ b/packages/ci/dist/index.js @@ -48620,16 +48620,23 @@ var PROPOSALS_HEADING = "#### Active proposals touching this change"; var PROPOSALS_NOTE = "These are not yet ratified and do not bind this change:"; var HISTORY_HEADING = "#### Historical records that once covered this change"; var HISTORY_NOTE = "These no longer bind this change, and are listed for context only:"; +var MARKER_SCAN_HEADING = "#### Inbound marker scan incomplete"; +var UNRESOLVED_MARKERS_HEADING = "#### Unresolved inbound markers"; var MAX_GOVERNING = 50; var MAX_DECLARATIONS = 10; +var MAX_MARKER_FINDINGS = 10; var MAX_COMMENT_CHARS = 65536; var TRUNCATION_NOTICE = "- …output truncated to fit GitHub’s comment size limit; run `adr check` locally for the complete result."; var MAX_FINDING_FIELD_CHARS = 256; var MAX_FINDING_MESSAGE_CHARS = 1024; +var MAX_MARKER_MESSAGE_CHARS = 512; function changedRecordFindings(outcome) { const changed = new Set(outcome.changedRecords); return outcome.findings.filter((finding) => finding.field !== "marker" && finding.path !== undefined && changed.has(finding.path)); } +function unresolvedMarkerFindings(outcome) { + return outcome.findings.filter((finding) => finding.rule === "dangling-marker" || finding.rule === "marker-unresolvable"); +} function code(value) { const safe = value.replace(/[\u0000-\u001f\u007f]/g, (char) => `\\x${char.charCodeAt(0).toString(16).padStart(2, "0")}`); let longestRun = 0; @@ -48651,6 +48658,56 @@ function renderFindingLine(finding) { const message = boundedDetail(finding.message, MAX_FINDING_MESSAGE_CHARS, "message"); return `- ${where} — ${code(finding.rule)}${field}: ${message}`; } +function renderMarkerFindingLine(finding) { + const where = finding.path ? code(finding.path) : "(unknown path)"; + const message = boundedDetail(finding.message, MAX_MARKER_MESSAGE_CHARS, "message"); + return `- ${where} — ${code(finding.rule)}: ${code(message)}`; +} +function countLabel(count, singular, plural = `${singular}s`) { + return `${count} ${count === 1 ? singular : plural}`; +} +function renderMarkerScanHealth(outcome) { + const report = outcome.markerScan; + if (!report) + return []; + const unavailable = report.counts.absent + report.counts.unreadable + report.counts["out-of-tree"] + report.counts.skipped; + if (unavailable === 0) + return []; + const reasons = []; + if (report.counts.absent > 0) { + reasons.push(countLabel(report.counts.absent, "absent", "absent")); + } + if (report.counts.unreadable > 0) { + reasons.push(countLabel(report.counts.unreadable, "unreadable", "unreadable")); + } + if (report.counts["out-of-tree"] > 0) { + reasons.push(countLabel(report.counts["out-of-tree"], "outside the worktree", "outside the worktree")); + } + if (report.counts.skipped > 0) { + reasons.push(`${countLabel(report.counts.skipped, "skipped", "skipped")} by the ${report.limit}-file scan cap`); + } + return [ + MARKER_SCAN_HEADING, + "", + `Could not inspect ${countLabel(unavailable, "changed file")} for inbound \`@adr\` markers: ${reasons.join(", ")}.`, + "Marker-derived governance may be incomplete." + ]; +} +function renderUnresolvedMarkers(findings2) { + if (findings2.length === 0) + return []; + const lines = [ + UNRESOLVED_MARKERS_HEADING, + "", + `${countLabel(findings2.length, "marker claim")} could not be resolved against this decision log:`, + ...findings2.slice(0, MAX_MARKER_FINDINGS).map(renderMarkerFindingLine) + ]; + const remaining = findings2.length - Math.min(findings2.length, MAX_MARKER_FINDINGS); + if (remaining > 0) { + lines.push(`- …and ${countLabel(remaining, "more unresolved marker finding")}; run \`adr check\` locally for the complete result.`); + } + return lines; +} function renderDecisionLines(decision, withStatus) { const status = withStatus ? ` _(${decision.status})_` : ""; const successor = decision.supersededBy ? ` — superseded by **${decision.supersededBy}**` : ""; @@ -48701,6 +48758,8 @@ function renderComment(outcome) { const findings2 = changedRecordFindings(outcome); const errors4 = findings2.filter((finding) => finding.severity === "error"); const warnings = findings2.filter((finding) => finding.severity === "warn"); + const markerScanHealth = renderMarkerScanHealth(outcome); + const markerFindings = renderUnresolvedMarkers(unresolvedMarkerFindings(outcome)); if (errors4.length > 0) { lines.push("#### ⚠️ Validation errors on changed records", ""); lines.push("These changed records fail validation and must be fixed:"); @@ -48708,6 +48767,12 @@ function renderComment(outcome) { lines.push(renderFindingLine(finding)); lines.push(""); } + if (markerScanHealth.length > 0) { + lines.push(...markerScanHealth, ""); + } + if (markerFindings.length > 0) { + lines.push(...markerFindings, ""); + } if (outcome.governedBy.length === 0) { lines.push(EMPTY_STATE); } else if (outcome.governing.length === 0) { diff --git a/packages/ci/src/comment.ts b/packages/ci/src/comment.ts index 989740d9..96e8cc97 100644 --- a/packages/ci/src/comment.ts +++ b/packages/ci/src/comment.ts @@ -23,6 +23,8 @@ const PROPOSALS_HEADING = '#### Active proposals touching this change'; const PROPOSALS_NOTE = 'These are not yet ratified and do not bind this change:'; const HISTORY_HEADING = '#### Historical records that once covered this change'; const HISTORY_NOTE = 'These no longer bind this change, and are listed for context only:'; +const MARKER_SCAN_HEADING = '#### Inbound marker scan incomplete'; +const UNRESOLVED_MARKERS_HEADING = '#### Unresolved inbound markers'; // Display cap for a pathological governing list. The underlying set is never // trimmed semantically (R6) — this only shortens what is rendered. @@ -34,6 +36,11 @@ const MAX_GOVERNING = 50; // Bounding what is rendered is what keeps that content out of the body budget below. const MAX_DECLARATIONS = 10; +// Marker findings are authored by the pull request and can outnumber the useful +// governance detail. Show enough examples to make the problem actionable, then +// collapse the rest into one deterministic count. +const MAX_MARKER_FINDINGS = 10; + /** * GitHub rejects a comment body over 65,536 characters with a 422. That is not a * permission error, so it would propagate out of the Action and fail the job — which @@ -48,6 +55,7 @@ const TRUNCATION_NOTICE = // detail must not make that whole line too large for the body limiter to retain. const MAX_FINDING_FIELD_CHARS = 256; const MAX_FINDING_MESSAGE_CHARS = 1024; +const MAX_MARKER_MESSAGE_CHARS = 512; function changedRecordFindings(outcome: CheckOutcome): Finding[] { const changed = new Set(outcome.changedRecords); @@ -57,6 +65,13 @@ function changedRecordFindings(outcome: CheckOutcome): Finding[] { ); } +function unresolvedMarkerFindings(outcome: CheckOutcome): Finding[] { + return outcome.findings.filter( + (finding) => + finding.rule === 'dangling-marker' || finding.rule === 'marker-unresolvable', + ); +} + /** * Render a value as an inline code span it cannot escape. * @@ -97,6 +112,79 @@ function renderFindingLine(finding: Finding): string { return `- ${where} — ${code(finding.rule)}${field}: ${message}`; } +function renderMarkerFindingLine(finding: Finding): string { + const where = finding.path ? code(finding.path) : '(unknown path)'; + const message = boundedDetail( + finding.message, + MAX_MARKER_MESSAGE_CHARS, + 'message', + ); + return `- ${where} — ${code(finding.rule)}: ${code(message)}`; +} + +function countLabel(count: number, singular: string, plural = `${singular}s`): string { + return `${count} ${count === 1 ? singular : plural}`; +} + +function renderMarkerScanHealth(outcome: CheckOutcome): string[] { + const report = outcome.markerScan; + if (!report) return []; + + const unavailable = + report.counts.absent + + report.counts.unreadable + + report.counts['out-of-tree'] + + report.counts.skipped; + if (unavailable === 0) return []; + + const reasons: string[] = []; + if (report.counts.absent > 0) { + reasons.push(countLabel(report.counts.absent, 'absent', 'absent')); + } + if (report.counts.unreadable > 0) { + reasons.push(countLabel(report.counts.unreadable, 'unreadable', 'unreadable')); + } + if (report.counts['out-of-tree'] > 0) { + reasons.push( + countLabel( + report.counts['out-of-tree'], + 'outside the worktree', + 'outside the worktree', + ), + ); + } + if (report.counts.skipped > 0) { + reasons.push( + `${countLabel(report.counts.skipped, 'skipped', 'skipped')} by the ${report.limit}-file scan cap`, + ); + } + + return [ + MARKER_SCAN_HEADING, + '', + `Could not inspect ${countLabel(unavailable, 'changed file')} for inbound \`@adr\` markers: ${reasons.join(', ')}.`, + 'Marker-derived governance may be incomplete.', + ]; +} + +function renderUnresolvedMarkers(findings: readonly Finding[]): string[] { + if (findings.length === 0) return []; + + const lines = [ + UNRESOLVED_MARKERS_HEADING, + '', + `${countLabel(findings.length, 'marker claim')} could not be resolved against this decision log:`, + ...findings.slice(0, MAX_MARKER_FINDINGS).map(renderMarkerFindingLine), + ]; + const remaining = findings.length - Math.min(findings.length, MAX_MARKER_FINDINGS); + if (remaining > 0) { + lines.push( + `- …and ${countLabel(remaining, 'more unresolved marker finding')}; run \`adr check\` locally for the complete result.`, + ); + } + return lines; +} + /** * One decision as a bullet, annotated with its status and — for superseded records — * the successor that replaced it, so a reviewer is never shown a bare record id and @@ -169,6 +257,8 @@ export function renderComment(outcome: CheckOutcome): string { const findings = changedRecordFindings(outcome); const errors = findings.filter((finding) => finding.severity === 'error'); const warnings = findings.filter((finding) => finding.severity === 'warn'); + const markerScanHealth = renderMarkerScanHealth(outcome); + const markerFindings = renderUnresolvedMarkers(unresolvedMarkerFindings(outcome)); // Validation is the blocking result of this Action, so keep it ahead of the // potentially large governance detail. If the body must be truncated, a reviewer @@ -180,6 +270,15 @@ export function renderComment(outcome: CheckOutcome): string { lines.push(''); } + // These advisory sections are bounded and follow blocking validation so + // pull-request-authored marker content cannot displace changed-record errors. + if (markerScanHealth.length > 0) { + lines.push(...markerScanHealth, ''); + } + if (markerFindings.length > 0) { + lines.push(...markerFindings, ''); + } + if (outcome.governedBy.length === 0) { lines.push(EMPTY_STATE); } else if (outcome.governing.length === 0) { diff --git a/packages/ci/test/action.test.ts b/packages/ci/test/action.test.ts index 9e41f8a7..9ea59e0e 100644 --- a/packages/ci/test/action.test.ts +++ b/packages/ci/test/action.test.ts @@ -121,6 +121,10 @@ describe('runAction (end to end with a fake client)', () => { expect(result.failed).toBe(false); expect(client.created[0]).toContain('**0001** — Guard marker-owned code'); expect(client.created[0]).toContain('declared by `src/owned.ts:1` (`@adr 0001`)'); + expect(client.created[0]).toContain('#### Inbound marker scan incomplete'); + expect(client.created[0]).toContain( + 'Could not inspect 1 changed file for inbound `@adr` markers: 1 absent.', + ); expect(logger.info.join('\n')).toContain( 'marker scan: 1 scanned, 1 absent, 0 unreadable, 0 out-of-tree, 1 truncated, 0 skipped', ); @@ -156,7 +160,7 @@ describe('runAction (end to end with a fake client)', () => { expect(result.outcome?.governing.map((decision) => decision.recordId)).toEqual(['0001']); }); - test('keeps dangling markers non-failing and out of the focused PR comment', async () => { + test('keeps dangling markers non-failing and reports them in the PR comment', async () => { const root = await resetTestDir(DIR_NAME); await mkdir(join(root, 'docs/adr'), { recursive: true }); await writeText(join(root, 'src/dangling.ts'), '// @adr 9999\n'); @@ -167,7 +171,8 @@ describe('runAction (end to end with a fake client)', () => { expect(result.failed).toBe(false); expect(result.outcome?.ok).toBe(true); expect(result.outcome?.findings.map((finding) => finding.rule)).toContain('dangling-marker'); - expect(client.created[0]).not.toContain('dangling-marker'); + expect(client.created[0]).toContain('#### Unresolved inbound markers'); + expect(client.created[0]).toContain('dangling-marker'); }); test('warns with the exact skipped paths when the marker scan cap is reached', async () => { diff --git a/packages/ci/test/comment-render.test.ts b/packages/ci/test/comment-render.test.ts index aabe68b9..95dd9879 100644 --- a/packages/ci/test/comment-render.test.ts +++ b/packages/ci/test/comment-render.test.ts @@ -79,22 +79,89 @@ describe('renderComment', () => { expect(body).not.toContain('**0002**'); }); - test('keeps marker reference warnings out of the focused PR comment', async () => { + test('reports a dangling marker after a healthy scan (#126)', async () => { const root = await seed(); - const outcome = await outcomeFor(root, ['docs/adr/0001-api.md']); - outcome.findings.push({ - rule: 'dangling-marker', - severity: 'warn', - message: 'Source marker does not resolve', - path: 'docs/adr/0001-api.md', - field: 'marker', - pattern: '9999', + const file = 'src/dangling.ts'; + await writeText(join(root, file), '// @adr 9999\n'); + const lint = await lintCorpus({ cwd: root, dir: 'docs/adr' }); + const markerScans = await readSourceMarkersBatch([file], root); + const outcome = checkChanges({ + lint, + changedFiles: [file], + dir: 'docs/adr', + markerScans, }); const body = renderComment(outcome); - expect(body).not.toContain('dangling-marker'); - expect(body).not.toContain('Source marker does not resolve'); + expect(outcome.markerScan?.counts).toEqual({ + scanned: 1, + absent: 0, + unreadable: 0, + 'out-of-tree': 0, + truncated: 0, + skipped: 0, + }); + expect(body).toContain('#### Unresolved inbound markers'); + expect(body).toContain('`src/dangling.ts`'); + expect(body).toContain('`dangling-marker`'); + expect(body).toContain('@adr 9999'); + expect(body).not.toContain('Inbound marker scan incomplete'); + }); + + test('reports could-not-look scan health separately from unresolved markers (#112)', async () => { + const root = await seed(); + const outcome = await outcomeFor(root, ['src/absent.ts', 'src/unreadable.ts', '../outside.ts']); + outcome.markerScan = { + totalCandidates: 5, + limit: 4, + counts: { + scanned: 1, + absent: 1, + unreadable: 1, + 'out-of-tree': 1, + truncated: 0, + skipped: 1, + }, + absentPaths: ['src/absent.ts'], + unreadablePaths: ['src/unreadable.ts'], + outOfTreePaths: ['../outside.ts'], + truncatedPaths: [], + skippedPaths: ['src/skipped.ts'], + }; + + const body = renderComment(outcome); + + expect(body).toContain('#### Inbound marker scan incomplete'); + expect(body).toContain( + 'Could not inspect 4 changed files for inbound `@adr` markers: 1 absent, 1 unreadable, 1 outside the worktree, 1 skipped by the 4-file scan cap.', + ); + expect(body).toContain('Marker-derived governance may be incomplete.'); + expect(body).not.toContain('#### Unresolved inbound markers'); + }); + + test('does not call a deliberately bounded header read incomplete', async () => { + const root = await seed(); + const outcome = await outcomeFor(root, ['src/large.ts']); + outcome.markerScan = { + totalCandidates: 1, + limit: 3000, + counts: { + scanned: 1, + absent: 0, + unreadable: 0, + 'out-of-tree': 0, + truncated: 1, + skipped: 0, + }, + absentPaths: [], + unreadablePaths: [], + outOfTreePaths: [], + truncatedPaths: ['src/large.ts'], + skippedPaths: [], + }; + + expect(renderComment(outcome)).not.toContain('Inbound marker scan incomplete'); }); }); @@ -233,6 +300,39 @@ describe('renderComment status awareness (#39)', () => { expect(body).toContain('frontmatter-parse'); }); + test('bounded marker findings cannot crowd out changed-record validation errors', async () => { + const root = await seed(); + const outcome = await outcomeFor(root, ['packages/api/src/server.ts']); + outcome.changedRecords = ['docs/adr/9999-broken.md']; + outcome.findings = [ + { + rule: 'frontmatter-parse', + severity: 'error', + message: 'Unterminated frontmatter', + path: 'docs/adr/9999-broken.md', + }, + ...Array.from({ length: 1000 }, (_, index) => ({ + rule: 'dangling-marker', + severity: 'warn' as const, + message: `Source marker "@adr ${String(index).padStart(4, '0')}" in src/${'x'.repeat(100)}/${index}.ts:1 does not resolve to a record in the corpus`, + path: `src/${'x'.repeat(100)}/${index}.ts`, + field: 'marker', + pattern: String(index).padStart(4, '0'), + })), + ]; + outcome.ok = false; + + const body = renderComment(outcome); + + expect(body.length).toBeLessThanOrEqual(65536); + expect(body).toContain('Validation errors on changed records'); + expect(body).toContain('docs/adr/9999-broken.md'); + expect(body).toContain('frontmatter-parse'); + expect(body.match(/ — `dangling-marker`:/g)).toHaveLength(10); + expect(body).toContain('…and 990 more unresolved marker findings'); + expect(renderComment(outcome)).toBe(body); + }); + test('an oversized finding message cannot crowd out its blocking path and rule', async () => { const root = await seed(); const outcome = await outcomeFor(root, ['packages/api/src/server.ts']); @@ -349,6 +449,30 @@ describe('renderComment status awareness (#39)', () => { expect(renderComment(outcome)).toContain('- ``docs/adr/0001-`bad`.md``'); }); + + test('marker paths and messages cannot escape their code spans', async () => { + const root = await seed(); + const outcome = await outcomeFor(root, ['src/a.ts']); + const hostile = 'src/x`[Approved](https://evil.example)`y\n.ts'; + outcome.findings = [ + { + rule: 'dangling-marker', + severity: 'warn', + message: `Source marker "@adr 9999" in ${hostile}:1 does not resolve`, + path: hostile, + field: 'marker', + pattern: '9999', + }, + ]; + + const body = renderComment(outcome); + + expect(body).toContain('``src/x`[Approved](https://evil.example)`y\\x0a.ts``'); + expect(body).toContain( + '``Source marker "@adr 9999" in src/x`[Approved](https://evil.example)`y\\x0a.ts:1 does not resolve``', + ); + expect(body).not.toContain('\ny\n.ts'); + }); }); test('a change matched only by non-accepted records says no accepted decision governs it', async () => { diff --git a/site/src/content/docs/ci.mdx b/site/src/content/docs/ci.mdx index 409aca8b..8607034b 100644 --- a/site/src/content/docs/ci.mdx +++ b/site/src/content/docs/ci.mdx @@ -43,6 +43,27 @@ The Action updates its existing comment on later pushes, so each pull request keeps one current governing-decisions comment. No additional token environment variable is required when using the default `GITHUB_TOKEN`. +The comment distinguishes two advisory marker states: + +- **Inbound marker scan incomplete** means the Action could not inspect one or + more changed files because they were absent, unreadable, outside the worktree, + or beyond the scan cap. Marker-derived governance may therefore be incomplete. +- **Unresolved inbound markers** means the Action successfully inspected a file + and found an `@adr` claim that does not bind in this decision log. It shows at + most 10 safely escaped examples and reports how many more were omitted. + +Neither state changes the Action verdict. Changed-record validation errors keep +priority if the comment reaches GitHub's size limit. + + + `v0` is a moving major tag, and it now resolves to a `v0.11.0` build. Pin the immutable `v0.11.0` tag or a commit SHA for maximum reproducibility.