From 0a994d838dbf7afceaf5987f1458d30efb2e8169 Mon Sep 17 00:00:00 2001 From: Eva Sarafianou Date: Fri, 16 Oct 2026 01:13:35 +0300 Subject: [PATCH] docs: scope AI documentation drafts to the merged change Draft PRs were titled by number, filled with the review loop, and restyling whole pages because pre-open review treated every line as new. Co-authored-by: Cursor --- .github/prompts/docs-writer.md | 43 ++++- .github/scripts/docs-ai/README.md | 4 +- .github/scripts/docs-ai/lib/diff.mjs | 175 ++++++++++++++++-- .github/scripts/docs-ai/lib/diff.test.mjs | 60 +++++- .github/scripts/docs-ai/prompts.test.mjs | 10 + .../scripts/docs-ai/review/persona-review.mjs | 8 +- .../scripts/docs-ai/writer/author-loop.mjs | 127 +++++++------ .../scripts/docs-ai/writer/draft-title.mjs | 23 +++ .github/scripts/docs-ai/writer/run.mjs | 7 +- .../scripts/docs-ai/writer/writer.test.mjs | 157 +++++++++++++++- .github/workflows/docs-writer-merged-pr.yml | 19 +- 11 files changed, 548 insertions(+), 85 deletions(-) create mode 100644 .github/scripts/docs-ai/writer/draft-title.mjs diff --git a/.github/prompts/docs-writer.md b/.github/prompts/docs-writer.md index ded5076f050..c6c1e50b10e 100644 --- a/.github/prompts/docs-writer.md +++ b/.github/prompts/docs-writer.md @@ -1,7 +1,7 @@ # Docs writer You draft Mattermost documentation pages on behalf of the technical writing team. -You receive untrusted PR evidence and a gap brief; you emit MDX file blocks only. +You receive untrusted PR evidence and a gap brief; you emit a summary fence and MDX file blocks only. ## Role @@ -21,6 +21,30 @@ when the brief asks for one; otherwise extend the named existing page. Ask: what can the reader do now that they could not before? Answer in one sentence and write only what that sentence requires. +## Editing an existing page + +When an existing page is supplied, change only the lines the brief requires. Copy every +other line through unchanged, including heading case, spacing, grammar, typos, and code +fence style. Conventions apply to the text you add. Reviewer feedback about a line you +did not add is out of scope; do not apply it. + +## How much to write + +Match the depth of the page you are editing and of the source pull request's release +note. Say what the reader can do, and any single constraint that changes which path +they take. + +Do not document exact error strings, bind addresses, firewall rules, port conflicts, or +packaging variables. Reviewer feedback is not evidence. If a reviewer asks for behaviour +the pull request description and the code diff do not state, leave it out. + +## Closing the brief + +Each recommended action is its own heading or procedure, at the same level as the +sibling sections on that page. A sentence inside another section does not close an +action. When the brief names more than one behaviour, document them in the order a +reader meets them. + ## Hard rules - Claims must map to evidence in the supplied PR metadata, description, code diff, or @@ -33,8 +57,8 @@ write only what that sentence requires. `docs/vendor/**`, or `docs/main/agents/docs/**`. - Do not edit changelogs, important upgrade notes, or version-archive pages unless the brief explicitly requires it. -- Follow `conventions.md` for frontmatter, plan availability, callouts, links, MDX - escaping, heading case, and voice. +- Follow `conventions.md` for text you add: frontmatter, plan availability, callouts, + links, MDX escaping, heading case, and voice. Do not restyle existing lines to match. ## Observability and diagnostics @@ -64,8 +88,17 @@ log formatting" when the meaning of the output is unchanged. ## Output format -Return one or more fenced code blocks, each preceded by a `path=` attribute naming the -destination relative to the repository root. No prose between blocks. +Return a summary fence, then one or more file fences. No other prose. + +```summary +- What the reader can do now, one behaviour per bullet. +``` + +The summary is the draft pull request description. Describe the documentation change. +Do not restate the gap analysis, and do not mention review feedback. + +Each file fence is preceded by a `path=` attribute naming the destination relative to +the repository root. Use a **four-backtick** outer fence so any three-backtick code samples inside the page do not close the block early: diff --git a/.github/scripts/docs-ai/README.md b/.github/scripts/docs-ai/README.md index d0f56d036d1..b135d1d6edd 100644 --- a/.github/scripts/docs-ai/README.md +++ b/.github/scripts/docs-ai/README.md @@ -111,7 +111,9 @@ out of the comment it wrote last time, so it can tell its own label from a human When a PR **merges** still carrying `Docs/Needed`, `docs-writer-merged-pr.yml` reads the sticky comment's recommended actions, authors MDX with the matching `author` personas, pre-reviews in-loop, and opens `docs/pr-` as a draft (authored by the Docs AI App). -Closing that draft flips `Docs/Needed` → `Docs/Done` on the originating PR. +The draft title comes from the source PR title. The draft body says what the +documentation change does. The pre-open review trail stays in `trail.json`, not the +body. Closing that draft flips `Docs/Needed` → `Docs/Done` on the originating PR. Three constraints to preserve if you change the gap label logic: diff --git a/.github/scripts/docs-ai/lib/diff.mjs b/.github/scripts/docs-ai/lib/diff.mjs index f17f2b18f63..bf2a0f77440 100644 --- a/.github/scripts/docs-ai/lib/diff.mjs +++ b/.github/scripts/docs-ai/lib/diff.mjs @@ -1,21 +1,23 @@ /* - * Synthesise an all-additions unified diff so pre-open persona review takes the - * same prompt shape as docs-review.yml's git diff. + * Diffs for persona review. + * + * additionsDiff marks every line as new. unifiedDiff compares an authored page + * with the on-disk original so pre-open review sees only the introduced change. + * A missing original is a new file and uses additionsDiff. */ +/** Trailing newline is not an extra empty line. */ +export function toLines(content) { + const text = String(content ?? ''); + if (text === '') return []; + const body = text.endsWith('\n') ? text.slice(0, -1) : text; + return body.split('\n'); +} + export function additionsDiff(files) { const chunks = []; for (const {path, content} of files) { - const lines = content === '' - ? [] - : (() => { - const body = content.endsWith('\n') ? content : `${content}\n`; - const parts = body.split('\n'); - // Trailing empty from the final newline — drop it so +++ counts match. - if (parts.at(-1) === '') parts.pop(); - return parts; - })(); - + const lines = toLines(content); const n = lines.length; chunks.push( `diff --git a/${path} b/${path}`, @@ -28,3 +30,152 @@ export function additionsDiff(files) { } return chunks.join('\n') + (chunks.length ? '\n' : ''); } + +const MAX_LCS_CELLS = 16_000_000; + +/** LCS edit script for two line arrays. `type` is ' ', '-', or '+'. */ +function lcsDiff(a, b) { + const n = a.length; + const m = b.length; + const dp = Array.from({length: n + 1}, () => new Uint32Array(m + 1)); + for (let i = n - 1; i >= 0; i--) { + const row = dp[i]; + const next = dp[i + 1]; + for (let j = m - 1; j >= 0; j--) { + row[j] = a[i] === b[j] ? next[j + 1] + 1 : Math.max(next[j], row[j + 1]); + } + } + + const ops = []; + let i = 0; + let j = 0; + while (i < n && j < m) { + if (a[i] === b[j]) { + ops.push({type: ' ', line: a[i]}); + i += 1; + j += 1; + } else if (dp[i + 1][j] >= dp[i][j + 1]) { + ops.push({type: '-', line: a[i]}); + i += 1; + } else { + ops.push({type: '+', line: b[j]}); + j += 1; + } + } + while (i < n) ops.push({type: '-', line: a[i++]}); + while (j < m) ops.push({type: '+', line: b[j++]}); + return ops; +} + +/** + * Edit script against the existing page. Shared prefix and suffix stay as + * context so a long page is not compared in full. A changed middle that is + * still too large to align is emitted as a straight replace. + */ +export function lineDiff(before, after) { + const a = toLines(before); + const b = toLines(after); + let start = 0; + const maxStart = Math.min(a.length, b.length); + while (start < maxStart && a[start] === b[start]) start += 1; + + let endA = a.length; + let endB = b.length; + while (endA > start && endB > start && a[endA - 1] === b[endB - 1]) { + endA -= 1; + endB -= 1; + } + + const prefix = a.slice(0, start).map((line) => ({type: ' ', line})); + const suffix = a.slice(endA).map((line) => ({type: ' ', line})); + const midA = a.slice(start, endA); + const midB = b.slice(start, endB); + const mid = midA.length * midB.length > MAX_LCS_CELLS + ? [ + ...midA.map((line) => ({type: '-', line})), + ...midB.map((line) => ({type: '+', line})), + ] + : lcsDiff(midA, midB); + return [...prefix, ...mid, ...suffix]; +} + +function formatHunks(ops, context = 3) { + const changeIdx = []; + for (let i = 0; i < ops.length; i++) { + if (ops[i].type !== ' ') changeIdx.push(i); + } + if (changeIdx.length === 0) return ''; + + const ranges = []; + let start = changeIdx[0]; + let end = changeIdx[0]; + for (const idx of changeIdx.slice(1)) { + if (idx <= end + context * 2 + 1) { + end = idx; + } else { + ranges.push([start, end]); + start = idx; + end = idx; + } + } + ranges.push([start, end]); + + const oldAt = []; + const newAt = []; + let oldLine = 1; + let newLine = 1; + for (let i = 0; i < ops.length; i++) { + oldAt[i] = oldLine; + newAt[i] = newLine; + if (ops[i].type !== '+') oldLine += 1; + if (ops[i].type !== '-') newLine += 1; + } + + const parts = []; + for (const [changeStart, changeEnd] of ranges) { + const from = Math.max(0, changeStart - context); + const to = Math.min(ops.length - 1, changeEnd + context); + const slice = ops.slice(from, to + 1); + let oldCount = 0; + let newCount = 0; + for (const op of slice) { + if (op.type !== '+') oldCount += 1; + if (op.type !== '-') newCount += 1; + } + let oldStart = oldAt[from]; + let newStart = newAt[from]; + if (oldCount === 0) oldStart = Math.max(0, oldStart - 1); + if (newCount === 0) newStart = Math.max(0, newStart - 1); + const body = slice.map((op) => `${op.type}${op.line}`).join('\n'); + parts.push(`@@ -${oldStart},${oldCount} +${newStart},${newCount} @@\n${body}`); + } + return parts.join('\n'); +} + +/** + * Unified diff of authored pages against on-disk originals. + * Unchanged files are omitted. Missing originals are new files. + */ +export function unifiedDiff(files, originals = []) { + const byPath = new Map((originals || []).map((o) => [o.path, o.content ?? ''])); + const chunks = []; + for (const file of files || []) { + if (!byPath.has(file.path)) { + const added = additionsDiff([file]); + chunks.push(added.endsWith('\n') ? added.slice(0, -1) : added); + continue; + } + const hunks = formatHunks(lineDiff(byPath.get(file.path), file.content)); + if (!hunks) continue; + chunks.push( + [ + `diff --git a/${file.path} b/${file.path}`, + `--- a/${file.path}`, + `+++ b/${file.path}`, + hunks, + ].join('\n'), + ); + } + if (chunks.length === 0) return ''; + return `${chunks.join('\n')}\n`; +} diff --git a/.github/scripts/docs-ai/lib/diff.test.mjs b/.github/scripts/docs-ai/lib/diff.test.mjs index dda45fb6078..3d82b12f2f9 100644 --- a/.github/scripts/docs-ai/lib/diff.test.mjs +++ b/.github/scripts/docs-ai/lib/diff.test.mjs @@ -1,6 +1,6 @@ import {test} from 'node:test'; import assert from 'node:assert/strict'; -import {additionsDiff} from './diff.mjs'; +import {additionsDiff, unifiedDiff} from './diff.mjs'; function hunkHeader(diff) { const m = diff.match(/@@ -0,0 \+(\d+)(?:,(\d+))? @@/); @@ -50,3 +50,61 @@ test('additionsDiff joins multiple files as separate chunks', () => { test('additionsDiff returns empty string for no files', () => { assert.equal(additionsDiff([]), ''); }); + +test('unifiedDiff shows an insertion and leaves distant lines out of the hunk', () => { + const path = 'docs/main/administration-guide/manage/command-line-tools.mdx'; + const before = ['## Compliance export', '', 'Existing export text.', '', '## Server operations', ''].join('\n'); + const after = [ + '## Compliance export', + '', + 'Existing export text.', + '', + '## Read the bundled documentation', + '', + 'Open /_documentation/ on a running server.', + '', + '## Server operations', + '', + ].join('\n'); + const diff = unifiedDiff([{path, content: after}], [{path, content: before}]); + assert.match(diff, /^\+\+\+ b\/docs\/main/m); + assert.doesNotMatch(diff, /\/dev\/null/); + assert.match(diff, /\+## Read the bundled documentation/); + assert.match(diff, /\+Open \/_documentation\/ on a running server\./); + assert.doesNotMatch(diff, /\+## Compliance export/); + assert.doesNotMatch(diff, /-## Compliance export/); +}); + +test('unifiedDiff still reports a real edit away from the insertion', () => { + const path = 'docs/main/a.mdx'; + const before = ['## Compliance Export', '', 'Body.', '', '## Tail', ''].join('\n'); + const after = ['## Compliance export', '', 'Body.', '', '## Tail', '', 'Added.', ''].join('\n'); + const diff = unifiedDiff([{path, content: after}], [{path, content: before}]); + assert.match(diff, /-## Compliance Export/); + assert.match(diff, /\+## Compliance export/); + assert.match(diff, /\+Added\./); +}); + +test('unifiedDiff treats a missing original as a new file', () => { + const file = {path: 'docs/main/new.mdx', content: 'Hello\n'}; + const diff = unifiedDiff([file], []); + assert.equal(diff, additionsDiff([file])); +}); + +test('unifiedDiff omits a file that did not change', () => { + const path = 'docs/main/a.mdx'; + const content = 'Same\n'; + assert.equal(unifiedDiff([{path, content}], [{path, content}]), ''); +}); + +test('unifiedDiff on a long page reports the insertion and not the unchanged head', () => { + const path = 'docs/main/a.mdx'; + const head = Array.from({length: 5000}, (_, i) => `line ${i}`); + const before = [...head, 'tail'].join('\n'); + const after = [...head.slice(0, 2500), '## New', ...head.slice(2500), 'tail'].join('\n'); + const diff = unifiedDiff([{path, content: `${after}\n`}], [{path, content: before}]); + assert.match(diff, /\+## New/); + assert.doesNotMatch(diff, /\+line 0\b/); + assert.doesNotMatch(diff, /\+line 4999\b/); + assert.doesNotMatch(diff, /-line 0\b/); +}); diff --git a/.github/scripts/docs-ai/prompts.test.mjs b/.github/scripts/docs-ai/prompts.test.mjs index 19fde7ba8b5..76edcd35869 100644 --- a/.github/scripts/docs-ai/prompts.test.mjs +++ b/.github/scripts/docs-ai/prompts.test.mjs @@ -81,3 +81,13 @@ test('writer prompt states scope and observability guidance', () => { assert.match(prompt, /USER_PASSWORD_CHANGED/); assert.match(prompt, /processWidgets/); }); + +test('writer prompt keeps a draft to the introduced change', () => { + const prompt = writerPrompt(); + assert.match(prompt, /Copy every\nother line through unchanged/); + assert.match(prompt, /exact error strings/); + assert.match(prompt, /own heading or procedure/); + assert.match(prompt, /```summary/); + assert.match(prompt, /Reviewer feedback is not evidence/); + assert.match(prompt, /does not close an\naction/); +}); diff --git a/.github/scripts/docs-ai/review/persona-review.mjs b/.github/scripts/docs-ai/review/persona-review.mjs index 8b3fc97b52b..8f2e73529fc 100644 --- a/.github/scripts/docs-ai/review/persona-review.mjs +++ b/.github/scripts/docs-ai/review/persona-review.mjs @@ -20,11 +20,13 @@ function readIfSet(name) { return path && existsSync(path) ? readFileSync(path, 'utf8') : null; } -export function buildReviewUserPrompt({diff, prTitle, prBody}) { +export function buildReviewUserPrompt({diff, prTitle, prBody, reviewNote, extra}) { const parts = [DATA_NOTICE, '']; if (prTitle) parts.push(block('pull-request-title', prTitle, {maxChars: 500}), ''); if (prBody) parts.push(block('pull-request-description', prBody, {maxChars: 6000}), ''); + if (extra) parts.push(extra, ''); + if (reviewNote) parts.push(reviewNote, ''); parts.push( block('diff', diff), @@ -56,6 +58,8 @@ export async function reviewPersona({ diff, prTitle, prBody, + reviewNote, + extra, model = MODEL, completeFn = complete, }) { @@ -64,7 +68,7 @@ export async function reviewPersona({ const {text, usage} = await completeFn({ model, system: reviewSystemBlocks(personaId), - userPrompt: buildReviewUserPrompt({diff, prTitle, prBody}), + userPrompt: buildReviewUserPrompt({diff, prTitle, prBody, reviewNote, extra}), maxTokens: 2048, temperature: 0.2, }); diff --git a/.github/scripts/docs-ai/writer/author-loop.mjs b/.github/scripts/docs-ai/writer/author-loop.mjs index d4c3aff5eae..860ff8b3f0f 100644 --- a/.github/scripts/docs-ai/writer/author-loop.mjs +++ b/.github/scripts/docs-ai/writer/author-loop.mjs @@ -1,7 +1,7 @@ import {existsSync, readFileSync} from 'node:fs'; import {join} from 'node:path'; import {complete, usageLine} from '../lib/anthropic.mjs'; -import {additionsDiff} from '../lib/diff.mjs'; +import {unifiedDiff} from '../lib/diff.mjs'; import { alwaysOnPersonaIds, authorSystemBlocks, @@ -11,6 +11,30 @@ import { REPO_ROOT as DEFAULT_ROOT, } from '../lib/personas.mjs'; import {DATA_NOTICE, block} from '../lib/untrusted.mjs'; + +const AUTHOR_REMINDER = [ + 'Copy unchanged lines through unchanged.', + 'Give each brief action its own heading or procedure. A sentence inside another section does not close it.', + 'Do not add error strings, bind or firewall detail, or packaging variables.', +].join(' '); + +/** Writer-loop only. docs-review.yml does not send this. */ +export const PREOPEN_REVIEW_NOTE = [ + 'This is a pre-open review of an AI documentation draft.', + 'REQUEST_CHANGES only when new text is factually wrong, or a required capability is missing as its own heading or procedure.', + 'A capability mentioned only inside another section is missing.', + 'Do not request exact error strings, bind addresses, firewall rules, port conflicts, or packaging variables. That is COMMENT at most, and only when the pull request evidence already states the behaviour.', + 'Do not request fixes to unchanged context lines.', +].join('\n'); + +const SUMMARY_RE = /`{3,}summary[ \t]*\n([\s\S]*?)\n`{3,}/; + +/** Summary fence from an author response. Empty when the model omits it. */ +export function parseSummaryBlock(text) { + const match = String(text || '').match(SUMMARY_RE); + if (!match) return ''; + return match[1].replace(//g, '').trim(); +} import {reviewPersona} from '../review/persona-review.mjs'; import {assertSafeMdx, assertVersionAnchors, mergeFiles, parseFileBlocks} from './files.mjs'; import {isAllowedPath} from './paths.mjs'; @@ -120,8 +144,8 @@ function buildAuthorUserPrompt({brief, groupPaths, input, revisionFeedback, exis } parts.push( revisionFeedback?.length - ? 'For paths with a current-draft block, revise that draft in place. Output the full updated file.' - : 'For paths with an existing-page block, edit that page in place (smallest change that closes the gap). Output the full updated file.', + ? 'For paths with a current-draft block, revise that draft in place. Output the full updated file. Keep every line the review did not require.' + : 'For paths with an existing-page block, edit that page in place. Copy every line you do not need to change through unchanged. Output the full updated file.', '', ); } @@ -130,11 +154,15 @@ function buildAuthorUserPrompt({brief, groupPaths, input, revisionFeedback, exis parts.push( block('reviewer-feedback', JSON.stringify(revisionFeedback, null, 2), {maxChars: 12_000}), '', - 'Revise the pages to address REQUEST_CHANGES feedback. Output the full file blocks again.', + 'Revise the text this draft added. Ignore feedback about pre-existing lines. Do not add behaviour that is not in the pull request evidence.', + 'Output the summary fence, then the full file blocks.', + AUTHOR_REMINDER, ); } else { parts.push( - 'Draft the MDX file(s) that close the documentation gap for the target paths. Output only the file blocks.', + 'Draft the MDX file(s) that close the documentation gap for the target paths.', + 'Output the summary fence, then the file blocks. No other prose.', + AUTHOR_REMINDER, ); } @@ -179,7 +207,7 @@ async function authorPass({personaId, groupPaths, brief, input, revisionFeedback if (files.length === 0) { throw new Error(`author ${personaId ?? 'neutral'} emitted no allowed file blocks`); } - return files; + return {files, summary: parseSummaryBlock(text)}; } async function selectReviewers({diff, completeFn}) { @@ -208,9 +236,21 @@ async function selectReviewers({diff, completeFn}) { } } -async function reviewFiles({files, input, completeFn}) { - const diff = additionsDiff(files); +function requiredCapabilities(brief) { + const actions = (brief?.actions ?? []).map((a) => String(a).trim()).filter(Boolean); + if (actions.length === 0) return ''; + return block('required-capabilities', actions.join('\n'), {maxChars: 4000}); +} + +async function reviewFiles({files, input, brief, completeFn}) { + const originals = loadExistingPages(input.repoRoot, files.map((f) => f.path)); + const diff = unifiedDiff(files, originals); + if (!diff.trim()) { + const paths = files.map((f) => f.path).join(', '); + throw new Error(`authored pages are identical to the existing files: ${paths}`); + } const reviewers = await selectReviewers({diff, completeFn}); + const extra = requiredCapabilities(brief); const results = await Promise.all( reviewers.map((personaId) => reviewPersona({ @@ -218,6 +258,8 @@ async function reviewFiles({files, input, completeFn}) { diff, prTitle: input.prTitle, prBody: input.prBody, + reviewNote: PREOPEN_REVIEW_NOTE, + extra, model: REVIEW_MODEL, completeFn, }), @@ -227,7 +269,8 @@ async function reviewFiles({files, input, completeFn}) { } /** - * Author → review → revise loop. Returns files + trail for the draft PR body. + * Author → review → revise loop. + * `summary` is the draft PR body. `trail` is kept for the workflow artifact only. */ export async function authorLoop({brief, input, completeFn = complete} = {}) { const targets = @@ -246,15 +289,18 @@ export async function authorLoop({brief, input, completeFn = complete} = {}) { let files = []; const trail = {iterations: [], openConcerns: []}; + const summaries = []; for (const group of groups) { - let groupFiles = await authorPass({ + let authored = await authorPass({ personaId: group.personaId, groupPaths: group.paths, brief, input, completeFn, }); + let groupFiles = authored.files; + let groupSummary = authored.summary; const initialMissing = missingGroupTargets(groupFiles, group.paths); if (initialMissing.length) { throw new Error( @@ -267,7 +313,7 @@ export async function authorLoop({brief, input, completeFn = complete} = {}) { let revision = 0; let lastResults = []; for (;;) { - const {results} = await reviewFiles({files: groupFiles, input, completeFn}); + const {results} = await reviewFiles({files: groupFiles, input, brief, completeFn}); lastResults = results; trail.iterations.push({ author: group.personaId ?? 'neutral', @@ -312,7 +358,7 @@ export async function authorLoop({brief, input, completeFn = complete} = {}) { basePages: groupFiles, completeFn, }); - const revisedMissing = missingGroupTargets(revised, group.paths); + const revisedMissing = missingGroupTargets(revised.files, group.paths); if (revisedMissing.length) { // Incomplete revision must not replace a complete prior pass. console.error( @@ -326,8 +372,8 @@ export async function authorLoop({brief, input, completeFn = complete} = {}) { break; } try { - assertVersionAnchors(revised, {milestoneVersion: input.milestoneVersion}); - assertSafeMdx(revised); + assertVersionAnchors(revised.files, {milestoneVersion: input.milestoneVersion}); + assertSafeMdx(revised.files); } catch (e) { // Dropped anchor / unsafe MDX must not replace a complete prior pass. console.error( @@ -340,9 +386,11 @@ export async function authorLoop({brief, input, completeFn = complete} = {}) { }); break; } - groupFiles = revised; + groupFiles = revised.files; + if (revised.summary) groupSummary = revised.summary; } + if (groupSummary) summaries.push(groupSummary); files = mergeFiles(files, groupFiles); console.error( `[author-loop] ${group.personaId ?? 'neutral'}: ${groupFiles.map((f) => f.path).join(', ')} ` + @@ -350,19 +398,16 @@ export async function authorLoop({brief, input, completeFn = complete} = {}) { ); } - return {files, trail}; + return {files, trail, summary: summaries.join('\n\n').trim()}; } -export function renderPrBody({sourcePr, trail, brief, milestoneVersion}) { +export function renderPrBody({sourcePr, summary, milestoneVersion}) { const lines = [ '', ``, '', `AI-drafted documentation for #${sourcePr}.`, '', - 'Authored by audience personas from the gap analysis sticky comment, then reviewed', - 'in-loop before this PR opened. Treat as a draft pending human editorial review.', - '', ]; if (!milestoneVersion) { @@ -373,41 +418,11 @@ export function renderPrBody({sourcePr, trail, brief, milestoneVersion}) { ); } - lines.push( - '## Gap brief', - '', - brief.summary || '_No summary recovered from the sticky comment._', - '', - ); - - if (brief.actions?.length) { - lines.push('### Recommended actions', '', ...brief.actions.map((a) => `- [ ] ${a}`), ''); - } - - lines.push('## Pre-open review trail', ''); - for (const iter of trail.iterations) { - lines.push( - `### Author \`${iter.author}\` — revision ${iter.revision}`, - '', - `Paths: ${iter.paths.map((p) => `\`${p}\``).join(', ')}`, - '', - ); - for (const r of iter.reviews) { - lines.push(`- **${r.persona}** — \`${r.verdict}\`: ${r.summary}`); - for (const f of r.feedback ?? []) lines.push(` - ${f}`); - } - lines.push(''); - } - - if (trail.openConcerns?.length) { - lines.push('## Open concerns after revision cap', ''); - for (const c of trail.openConcerns) { - lines.push(`- **${c.persona}**: ${c.summary}`); - for (const f of c.feedback ?? []) lines.push(` - ${f}`); - } - lines.push(''); - } - - lines.push(`Closes the docs gap filed against #${sourcePr}.`); + let change = String(summary || '') + .replace(//g, '') + .trim(); + if (change.length > 2000) change = `${change.slice(0, 2000).trimEnd()}...`; + lines.push('## Summary', '', change || '_The draft did not include a change summary._', ''); + lines.push('Pending human editorial review.'); return `${lines.join('\n')}\n`; } diff --git a/.github/scripts/docs-ai/writer/draft-title.mjs b/.github/scripts/docs-ai/writer/draft-title.mjs new file mode 100644 index 00000000000..34c875e4981 --- /dev/null +++ b/.github/scripts/docs-ai/writer/draft-title.mjs @@ -0,0 +1,23 @@ +const PREFIX = 'docs: AI draft for '; +const MAX_LEN = 240; + +/** + * Draft PR title from the merged PR's title. + * Strips a leading "docs:" so the prefix is not doubled, and drops control + * characters so the title stays a single git/GitHub line. + */ +export function draftTitle(sourceTitle) { + const cleaned = String(sourceTitle ?? '') + .replace(/[\u0000-\u001f\u007f]/g, ' ') + .replace(/\s+/g, ' ') + .trim() + .replace(/^docs:\s*/i, ''); + let subject = cleaned || 'documentation update'; + const room = MAX_LEN - PREFIX.length; + if (subject.length > room) { + const cut = subject.slice(0, room - 3); + const space = cut.lastIndexOf(' '); + subject = `${(space > 40 ? cut.slice(0, space) : cut).trimEnd()}...`; + } + return PREFIX + subject; +} diff --git a/.github/scripts/docs-ai/writer/run.mjs b/.github/scripts/docs-ai/writer/run.mjs index 8299616e8f2..47270d432ac 100644 --- a/.github/scripts/docs-ai/writer/run.mjs +++ b/.github/scripts/docs-ai/writer/run.mjs @@ -13,6 +13,7 @@ * Writes docs files into REPO_ROOT and emits: * .docs-ai/writer/trail.json * .docs-ai/writer/pr-body.md + * .docs-ai/writer/draft-title.txt * .docs-ai/writer/written.json */ @@ -22,6 +23,7 @@ import {findStickyComment} from '../lib/github.mjs'; import {MARKER} from '../gap/gap.mjs'; import {REPO_ROOT as DEFAULT_ROOT} from '../lib/personas.mjs'; import {authorLoop, renderPrBody} from './author-loop.mjs'; +import {draftTitle} from './draft-title.mjs'; import {versionFromMilestone, writeFiles} from './files.mjs'; import {briefFromGapResult, briefFromPrEvidence, parseGapBrief} from './gap-brief.mjs'; @@ -104,11 +106,12 @@ async function main() { repoRoot, }; - const {files, trail} = await authorLoop({brief, input}); + const {files, trail, summary} = await authorLoop({brief, input}); writeFileSync(join(outDir, 'trail.json'), `${JSON.stringify(trail, null, 2)}\n`); - const body = renderPrBody({sourcePr: pr, trail, brief, milestoneVersion}); + const body = renderPrBody({sourcePr: pr, summary, milestoneVersion}); writeFileSync(join(outDir, 'pr-body.md'), body); + writeFileSync(join(outDir, 'draft-title.txt'), `${draftTitle(input.prTitle)}\n`); if (process.env.DRY_RUN === '1') { writeFileSync(join(outDir, 'files.json'), `${JSON.stringify(files, null, 2)}\n`); diff --git a/.github/scripts/docs-ai/writer/writer.test.mjs b/.github/scripts/docs-ai/writer/writer.test.mjs index 89a0c82f814..81cdad60cac 100644 --- a/.github/scripts/docs-ai/writer/writer.test.mjs +++ b/.github/scripts/docs-ai/writer/writer.test.mjs @@ -1,5 +1,8 @@ import {test} from 'node:test'; import assert from 'node:assert/strict'; +import {mkdtempSync, mkdirSync, rmSync, writeFileSync} from 'node:fs'; +import {tmpdir} from 'node:os'; +import {dirname, join} from 'node:path'; import {additionsDiff} from '../lib/diff.mjs'; import {authorForPath, groupPathsByAuthor, REPO_ROOT} from '../lib/personas.mjs'; import { @@ -20,8 +23,12 @@ import { missingGroupTargets, normalizeDocsPath, parseMaxRevisions, + parseSummaryBlock, + PREOPEN_REVIEW_NOTE, + renderPrBody, authorLoop, } from './author-loop.mjs'; +import {draftTitle} from './draft-title.mjs'; import {MARKER, buildComment, decide} from '../gap/gap.mjs'; test('allowlist accepts hand-authored content roots', () => { @@ -482,7 +489,7 @@ test('authorLoop keeps prior files when a revision drops the version anchor', as }; } authorCalls += 1; - if (prompt.includes('Revise the pages')) { + if (prompt.includes('Revise the text this draft added')) { if (prompt.includes('current-draft path=' + path) && prompt.includes('From Mattermost v11.7')) { sawCurrentDraft = true; } @@ -503,3 +510,151 @@ test('authorLoop keeps prior files when a revision drops the version anchor', as assert.match(files[0].content, /From Mattermost v11\.7/); assert.ok(trail.openConcerns.some((c) => /Revision rejected/.test(c.summary))); }); + +test('draftTitle uses the source subject and stays a single line', () => { + assert.equal( + draftTitle('docs: ship offline documentation in the release tarball'), + 'docs: AI draft for ship offline documentation in the release tarball', + ); + assert.equal(draftTitle('Add the foo setting'), 'docs: AI draft for Add the foo setting'); + assert.equal(draftTitle('docs:\n\nship\rdocs'), 'docs: AI draft for ship docs'); + assert.equal(draftTitle(' '), 'docs: AI draft for documentation update'); + assert.equal(draftTitle(''), 'docs: AI draft for documentation update'); + const long = draftTitle(`docs: ${'word '.repeat(80)}`); + assert.ok(long.length <= 240); + assert.ok(long.startsWith('docs: AI draft for ')); + assert.ok(!long.includes('\n')); +}); + +test('parseSummaryBlock reads the summary fence and drops forged markers', () => { + const text = [ + '```summary', + '- Open /_documentation/ on a running server.', + '', + '```', + '', + '````mdx path=docs/main/a.mdx', + 'From Mattermost v12.1, hi.', + '````', + ].join('\n'); + const summary = parseSummaryBlock(text); + assert.match(summary, /_documentation/); + assert.doesNotMatch(summary, /docs-ai-source-pr/); + assert.equal(parseSummaryBlock('no fence'), ''); +}); + +test('renderPrBody describes the change and omits the review trail', () => { + const body = renderPrBody({ + sourcePr: '38957', + summary: '- On a running server, open /_documentation/.', + milestoneVersion: 'v12.1', + }); + assert.match(body, //); + assert.match(body, /## Summary/); + assert.match(body, /_documentation/); + assert.doesNotMatch(body, /## Gap brief/); + assert.doesNotMatch(body, /Pre-open review trail/); + assert.doesNotMatch(body, /Open concerns/); + assert.match(PREOPEN_REVIEW_NOTE, /exact error strings/); + + const missing = renderPrBody({sourcePr: '1', summary: '', milestoneVersion: ''}); + assert.match(missing, /NOT PRESENT — REQUIRES HUMAN JUDGMENT/); + assert.match(missing, /did not include a change summary/); + + const long = renderPrBody({ + sourcePr: '1', + summary: 'y'.repeat(2500), + milestoneVersion: 'v12.1', + }); + assert.match(long, /y{2000}\.\.\./); + assert.doesNotMatch(long, /y{2001}/); +}); + +test('authorLoop reviews only the inserted section of an existing page', async () => { + const dir = mkdtempSync(join(tmpdir(), 'docs-writer-')); + const rel = 'docs/main/administration-guide/manage/command-line-tools.mdx'; + const abs = join(dir, rel); + mkdirSync(dirname(abs), {recursive: true}); + const original = [ + '---', + 'title: "Command line tools"', + '---', + '', + '## Compliance export', + '', + 'Existing export text.', + '', + '## Server operations', + '', + 'Existing server text.', + '', + ].join('\n'); + writeFileSync(abs, original); + + const updated = original.replace( + '## Server operations', + [ + '## Read the bundled documentation', + '', + 'From Mattermost v12.1, open `/_documentation/` on a running server.', + '', + '## mattermost docs', + '', + 'From Mattermost v12.1, serve the bundle when the server is down.', + '', + '## Server operations', + ].join('\n'), + ); + + let reviewDiff = ''; + const completeFn = async ({userPrompt}) => { + const prompt = String(userPrompt || ''); + if (prompt.includes('Which reviewers apply?')) { + return {text: '{"personas": []}', usage: {}}; + } + if (prompt.includes('Review the diff above')) { + reviewDiff = prompt; + return { + text: JSON.stringify({verdict: 'APPROVE', summary: 'ok', feedback: []}), + usage: {}, + }; + } + return { + text: [ + '```summary', + '- On a running server, open /_documentation/.', + '- When the server is down, use mattermost docs.', + '```', + '', + '````mdx path=' + rel, + updated.trimEnd(), + '````', + ].join('\n'), + usage: {}, + }; + }; + + try { + const {files, summary} = await authorLoop({ + brief: { + summary: 'offline docs', + actions: ['Document the /_documentation/ route on a running server'], + targetPaths: [rel], + }, + input: {milestoneVersion: 'v12.1', repoRoot: dir, prTitle: 'docs: ship offline docs'}, + completeFn, + }); + + assert.match(summary, /mattermost docs/); + assert.match(files[0].content, /## Read the bundled documentation/); + assert.match(reviewDiff, /required-capabilities/); + assert.match(reviewDiff, /Document the \/_documentation\/ route/); + assert.match(reviewDiff, /exact error strings/); + assert.match(reviewDiff, /\+## Read the bundled documentation/); + assert.match(reviewDiff, /\+## mattermost docs/); + assert.doesNotMatch(reviewDiff, /\+## Compliance export/); + assert.doesNotMatch(reviewDiff, /\+title: "Command line tools"/); + } finally { + rmSync(dir, {recursive: true, force: true}); + } +}); diff --git a/.github/workflows/docs-writer-merged-pr.yml b/.github/workflows/docs-writer-merged-pr.yml index f5379a67c5e..25fc2432dd9 100644 --- a/.github/workflows/docs-writer-merged-pr.yml +++ b/.github/workflows/docs-writer-merged-pr.yml @@ -237,7 +237,6 @@ jobs: id: push env: BRANCH: ${{ steps.pr.outputs.branch }} - PR_NUMBER: ${{ steps.pr.outputs.pr_number }} GH_TOKEN: ${{ steps.app-token.outputs.token }} run: | set -euo pipefail @@ -248,7 +247,13 @@ jobs: echo "::error::Writer produced no file changes." exit 1 fi - printf 'docs: AI draft for #%s\n' "$PR_NUMBER" | git commit -F - + title_file=".docs-ai/writer/draft-title.txt" + if [ ! -s "$title_file" ]; then + echo "::error::Writer did not emit a draft title." + exit 1 + fi + TITLE=$(tr -d '\n' < "$title_file") + printf '%s\n' "$TITLE" | git commit -F - if git ls-remote --exit-code origin "refs/heads/${BRANCH}" >/dev/null 2>&1; then git push origin "HEAD:refs/heads/${BRANCH}" --force-with-lease else @@ -260,17 +265,21 @@ jobs: env: GH_TOKEN: ${{ steps.app-token.outputs.token }} BRANCH: ${{ steps.pr.outputs.branch }} - PR_NUMBER: ${{ steps.pr.outputs.pr_number }} REVIEWER: ${{ steps.pr.outputs.reviewer }} run: | set -euo pipefail EXISTING=$(gh pr list --head "$BRANCH" --state open --json number,url --jq 'first // empty') - TITLE="docs: AI draft for #${PR_NUMBER}" + title_file=".docs-ai/writer/draft-title.txt" + if [ ! -s "$title_file" ]; then + echo "::error::Writer did not emit a draft title." + exit 1 + fi + TITLE=$(tr -d '\n' < "$title_file") if [ -n "$EXISTING" ]; then NUM=$(echo "$EXISTING" | jq -r .number) URL=$(echo "$EXISTING" | jq -r .url) - gh pr edit "$NUM" --body-file .docs-ai/writer/pr-body.md + gh pr edit "$NUM" --title "$TITLE" --body-file .docs-ai/writer/pr-body.md else URL=$(gh pr create \ --base master \