api-docs skill: dual-purpose docs add + review (references + scripts) - #4242
Merged
Conversation
…ng + eval harness (P0–P2) Upgrade the api-docs skill into a dual-purpose (add + review) skill that edits mdoc XML directly, dropping the legacy extract/merge JSON round-trip. P0 — Skill scaffolding: - SKILL.md slimmed from a 377-line monolith to a ~95-line router. - workflows/ (add, review, scope-resolution, validation) and agents/ (writer, reviewer-factual/-examples/-quality, review-synthesizer), each agent carrying a one-line per-role Model: header. - references/obsolete-api-map.md (canonical obsolete→replacement table the linter parses) and assets/scope-aliases.yml (curated semantic scopes). - DRY: reviewers point at references instead of restating facts. P1 — Tooling (scripts/docs-tool.ps1): - Drop extract/merge; add resolve-scope, a deterministic linter (spelling, repeated-word, obsolete-in-example, cref/xref, empty/placeholder, accessor-verb), and a post-edit structural validator (well-formed + signature-count preserved + only <Docs> changed). Full-corpus lint: 406 files, 33 findings. P2 — Eval harness (eval/): - Synthetic lint-clean fixtures, mutations.json (12 seeded defects + 2 controls), inject.ps1 (writes only to gitignored output/eval/, refuses under docs/), score.ps1 (recall/precision vs answer key + controls), and run.ps1 (one-command lint self-test, gated recall 1.0 / precision 1.0). - README documents the harness plus the per-role model bake-off and the CI routing probe. Aligned the finding contract end to end: severity vocab CRITICAL/IMPORTANT/ MINOR, and reviewer-factual emits factual / fabricated-member class tokens that the scorer matches. Deferred for review (cross-repo / premium-model): p1b auto-api-docs-writer rewrite in the docs submodule, and P3–P6 actual doc reviews. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…OCS_DIR) The auto-api-docs-writer gh-aw workflow runs with the docs repo as the *primary* checkout and SkiaSharp cloned as a secondary repo, so docs-tool.ps1's assumption that docs live at <skiasharp>/docs (and that git baselines/diffs run there) breaks in that sandbox. Add two optional env overrides, both defaulting to today's in-repo submodule layout (no behavior change locally): - DOCS_GIT_ROOT — git repo used for `new`/`changed` diffs and the `validate` baseline (`git show HEAD:<rel>`). Defaults to <RepoRoot>/docs. - DOCS_DIR — directory scanned for the .xml files. Defaults to <RepoRoot>/docs/SkiaSharpAPI. $RepoRoot still points at the SkiaSharp checkout, so source lookups (binding/) keep working while docs come from elsewhere. Get-RelToDocs now strips $DocsGit so the git-relative path is correct under either layout. Verified: `lint all` (406/33) and the eval self-test (recall 1.0 / precision 1.0) unchanged on the default path; and against a simulated docs-as-primary git repo, `resolve-scope new` detects changed files and `validate new` resolves the baseline (docs-only, signatures preserved). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📦 Try the packages from this PRWarning Do not run these scripts without first reviewing the code in this PR. Step 1 — Download the packages bash / macOS / Linux: curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4242PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4242"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4242/packages --name skiasharp-pr-4242More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4242 |
Contributor
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
Ran the eval per-role bake-off (k=3) across gpt-5.5, claude-opus-4.6, claude-opus-4.5, claude-sonnet-4.6 for the factual reviewer role, scored against the seeded llm answer key (wrong default, ARGB->RGBA byte order, fabricated member) on a blind review tree. gpt-5.5 is the only candidate that substantively caught the byte-order contradiction and led on both mean recall (0.89) and precision (0.75), so reviewer-factual now routes to gpt-5.5. The bake-off also exposed a false-positive pattern (models asserting documented defaults 'should be 0' without reading source); reviewer-factual.md now requires a source read before flagging a default and marks unverifiable defaults UNVERIFIED. - agents/reviewer-factual.md: Model -> gpt-5.5; tighten defaults guidance - workflows/review.md: model table + bake-off snapshot - eval/bakeoff-results.md: full results, routing probe, harness limitations - eval/README.md: link to results Deterministic linter gate unchanged (recall 9/9=1.0, precision 1.0). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hinery)
The gh-aw sandbox does not honor per-sub-agent model routing, so the fancy
orchestration was dead weight. Simplify to the standard agentskills.io layout
(SKILL.md + references/ + scripts/ + assets/), run by a single agent:
- Fold agents/writer.md + workflows/add.md -> references/adding.md.
- Fold the three reviewer agents + synthesizer + workflows/review.md ->
references/reviewing.md (checks merged; no per-role model table, no bake-off).
- Move workflows/{scope-resolution,validation}.md -> references/.
- Remove agents/, workflows/, eval/ dirs and the per-role Model: headers.
- SKILL.md becomes a thin router to references + scripts (no orchestration,
no sub-agent fan-out, no eval section).
Prompt/methodology content is preserved verbatim in references; only the
multi-agent + per-model + eval scaffolding is removed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattleibow
changed the title
Curated group: aliases were a hand-maintained file list that drifts as new types ship. A capable model expands a plain-English theme into files more accurately. Remove assets/scope-aliases.yml, the mini-YAML reader, and the group: selector from docs-tool.ps1; for a semantic theme, resolve all (or a ns:) and select matching files. Deterministic selectors (file/type/ns/match/ new/changed/all) are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…iles Nobody crafts type:/ns:/match: selectors — the user says 'review the font docs' and the model must figure out which files that means. Collapse docs-tool.ps1 resolve-scope to a plain inventory: all | new (placeholders) | changed | file:PATH. The model reads 'all' and picks theme matches itself; 'new' drives 'do all adds'. Removed type:/ns:/match: cases, the fuzzy match/confirm machinery, and the -Confirm parameter. Reframed scope-resolution.md, reviewing.md, adding.md and SKILL.md around the two real intents (add everything missing / review what the user describes). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop the confusing double negatives (no selector grammar / no curated group / no sub-agent fan-out / no JSON round-trip) that referenced concepts a reader never sees. State positively: the user asks in plain language and you pick the files; resolve-scope lists all/new/changed/file; one agent edits the XML. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…he procedures After removing the selector grammar the file was ~85% duplication: the script self-documents the inventory modes, and both procedures already inline the sharding/incremental note and the theme-pick step. Keep the one useful piece (the font-theme expansion example + the source:NONE note) in reviewing.md step 1 and adding.md step 2; remove the pointers from SKILL.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the three deterministic docs QA gates out of the skill's PowerShell script and into scripts/infra/docs/docs.cake as sibling Cake tasks, exposed from build.cake. The skill is now references-only (no scripts/ dir); SKILL.md and the procedure refs call `dotnet cake --target=docs-* --scope=...`. Paths, the obsolete-map, and the review base are overridable via --args or the DOCS_GIT_ROOT / DOCS_DIR / OBSOLETE_MAP / DOCS_REVIEW_BASE env vars for the gh-aw inverted-checkout sandbox. The port is output-identical to the ps1 (verified: `resolve-scope all` and `validate all` are byte-for-byte equal across 406 files, and every validate failure path matches) with two intentional differences: - Placeholder matching is now case-sensitive, which removes 4 false positives where the ps1's case-insensitive `-match` flagged the prose phrase "to be added" (e.g. "elements to be added to the current path") as a "To be added." stub. mdoc only ever emits the exact "To be added." placeholder. - repeated-word findings report the element name (e.g. <param>) instead of the PowerShell attribute-name property collision (<mode>); the message field is human-readable only and is not machine-parsed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collapse the separate docs-resolve-scope/docs-lint/docs-validate Cake tasks into the existing docs-format-docs target, which already loads and parses every doc XML. As it formats each type file it now runs the deterministic content checks too, so there is a single "format + check" pass instead of three tasks the workflow never called. Failure model: - Broken XML/CDATA (malformed-xml, broken-cdata) -> Error, build fails. These would break the published Learn site, so they must never ship. - Everything else (missing docs, accessor-verb, obsolete-in-example, spelling, repeated-word, empty-tag, cref/xref) -> Warning. Missing-doc coverage is already logged (now as warnings), so the lint call suppresses duplicate placeholder findings. The structural git-baseline validator is dropped: it never fit the "broken vs missing" model and conflicted with regeneration (signatures legitimately change when new APIs land); signatures are owned by mdoc and visible in the PR diff, and the malformed-xml error is the real "no broken XML ships" guarantee. Because missing docs are warnings, full regeneration (which emits all-stub docs) never fails. Remove the three tasks, their build.cake passthroughs, and the now-dead scope/git/validate helpers and config. Repoint the api-docs skill and references to the single target; rewrite validation.md around the new warning/error split. The auto-api-docs-writer workflow already calls only docs-format-docs, so its post-step now also gates broken XML. Tested: clean tree -> 406 files, 29 findings, 0 errors, exit 0; an injected escaped-xref CDATA -> [docs] broken-cdata Error, exit 1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Every member in the obsolete map is banned from code examples and the folded docs-format-docs linter emits a warning for each (broken XML is the only build-failing class), so the per-row error/warn Severity no longer routes anything. Drop the column from the table, simplify QaObsoleteMembers to collect all listed members, and update the reviewing reference to stop citing a map 'error' level (the source-level [Obsolete(..., error: true)] vs soft-obsolete CRITICAL/IMPORTANT distinction stays, since that lives in checklist.md). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
docs-format-docs already loads every file once with XDocument and reformats it; the lint was re-reading each type file a second time through XmlDocument/XPath. Fold the checks into the same pass, on the tree already in memory: - QaLintFile (re-parse + XPath) -> QaCheckDocs (operates on the live XElement), called from the format loop for the type Docs, each member, and each MemberGroup. QaProseSegments now walks XText/XCData via LINQ-to-XML. - Drop the malformed-xml class entirely: XDocument.Load at the top of the loop already throws on unparseable XML, so the second parse could never see it. Wrap that load to fail with the file name. QA_ERROR_CLASSES is now just broken-cdata. - broken-cdata is detected by node kind: a <xref: in an ordinary text node means the CDATA wrapper was destroyed; an escaped <xref: inside a CDATA node is the doubly-escaped form. Both fail the build. - Delete the now-dead XmlDocument helpers (QaLoadXml, QaAttr, QaMemberDocId). Behaviour-preserving: clean tree still reports 406 files / 29 findings / 0 errors; both broken-cdata forms still error and exit 1. validation.md updated to describe the parse-failure (throw) vs broken-cdata split. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
QaRelTo(root, abs) was only ever called by QaRelToRepo(abs) with the single QA_REPO_ROOT — the parameterised root was leftover generality from when a separate docs checkout root was imagined. Inline it into QaRelToRepo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cosmetic cleanup of the lint folded into docs-format-docs; no behaviour change (clean tree still 406 files / 29 findings / 0 errors, broken-cdata still exits 1): - Drop the Qa/QA_ prefix everywhere: QaCheckDocs -> CheckDocs, QaProseSegments -> ProseSegments, QaIsGenerated -> IsGeneratedDocFile, QaRelToRepo -> RelativeToRepo, QaObsoleteMembers -> ReadObsoleteMembers, QA_MISSPELL -> MISSPELLINGS, QA_CREF_PREFIXES -> CREF_PREFIXES, QA_OBSOLETE_MAP -> OBSOLETE_MAP_PATH, QA_ERRORS -> DocErrors. Drop the unused QA_REPO_ROOT (use ROOT_PATH.FullPath). - Move the consts + helpers above the tasks so the file reads top-down (helpers -> tasks) with no backward references. - Remove the QaEmit indirection and its QA_ERROR_CLASSES set: CheckDocs now calls Warning(...) at each check and Error(...)+DocErrors++ for the one fatal class (broken-cdata), keeping the same [docs] <class> | <file> | <docId> | <msg> line and the finding/error tallies for the summary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
More lint cleanup, no behaviour change (clean tree still 406/29/0, broken-cdata still exits 1): - CheckDocs no longer writes a script-global error counter. It takes ref int errors and bumps the caller's tally, so the count lives as a normal task-local (lintErrors) alongside lintedTypes/lintFindings. - Remove RelativeToRepo and just log full file paths in the [docs] lines and the parse-failure message. - Trim the oversized banner comment down to a plain four-line note matching the other helpers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The deterministic obsolete-in-example check matches obsolete members by name, so it flags every .DrawText/.MeasureText even on the modern non-obsolete SKFont/ SKCanvas overloads. In run 28411250198 the agent burned several turns on this and deleted a valid font.MeasureText(...) call to silence it. Tell reviewers to verify the exact overload is [Obsolete] before acting, and never remove a valid modern call for a linter false positive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The deterministic obsolete-in-example check in docs.cake matched obsolete members by name, so it flagged modern SKFont/SKCanvas text overloads that merely share a name with the deprecated ones (e.g. font.MeasureText, canvas.DrawText(..,SKFont,..)). That produced false positives and, worse, a contradictory skill: the map said a name was banned while reviewing.md told reviewers to ignore the warning. Resolve it by removing the deterministic obsolete-in-example check entirely (the ReadObsoleteMembers parser, the OBSOLETE_MAP_PATH input, and the per-fence regex) and making obsolete-api-map.md an unambiguous, overload-aware reference the model follows directly: - Section 1 lists members that are always obsolete (never use). - Section 2 lists methods whose name also exists on the modern API, with the rule that the deprecated form is the one without an SKFont argument (on SKPaint, or the SKCanvas overload with no SKFont); prefer the SKFont overload. This also serves as the preferred-overload selector for writers. All references that described a deterministic obsolete linter (SKILL.md, validation.md, checklist.md, patterns.md, skia-patterns.md, reviewing.md) now point at the reference and the reviewer judgement instead. Broken-XML/CDATA errors and the missing-doc and spelling warnings are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattleibow
added a commit
to mono/SkiaSharp-API-docs
that referenced
this pull request
Jun 30, 2026
…, thin trigger (#155) Harden auto-api-docs-writer: single Opus 4.7 agent, branch-safe clone, thin trigger (#155) Context: runs the api-docs skill from mono/SkiaSharp#4242 (skill is the source of truth) Hardens and simplifies the `auto-api-docs-writer` agentic (gh-aw) workflow so it reliably formats, fills, reviews, and opens a PR for API docs without clobbering branches or duplicating the docs tree. The workflow now carries only gh-aw wiring; the how-to-write-docs procedure lives in the skill. ~~ Single agent on Opus 4.7 ~~ engine.model -> claude-opus-4.7, and one agent does add + review + fix + validate + PR. The gh-aw sandbox does not honor per-sub-agent model routing (the task tool's model param is not plumbed to the API call — verified via the api-proxy token-usage log), so the multi-agent fan-out only added complexity and a no-PR failure mode when a terminal "fixer" sub-agent was backgrounded at end of turn. ~~ Branch safety and clone hygiene ~~ The host prepares a dedicated throwaway PR branch before the agent runs, so the agent never commits on (and recreate_ref never clobbers) the dispatch ref. The SkiaSharp clone uses --no-recurse-submodules + rm -rf + a file-count assertion and echoes the resolved HEAD, and the docs-tree symlink duplication is eliminated (workspace == linked) so the lint scans each file once. Git is quieted in the agent run. ~~ Configurable refs ~~ Adds workflow_dispatch inputs skiasharp_branch (where the skill lives), docs_base_branch (default main), and docs_head_branch (default automation/write-api-docs). The primary checkout and stub regeneration both pin to docs_base_branch (via FETCH_HEAD, so arbitrary/shallow branches work) and the PR targets it — keeping base, working tree, and stubs in agreement instead of reviewing the wrong base when dispatched on a feature branch. ~~ Cleanup ~~ Slims the workflow to a thin trigger, removes the stale review_scope input and the obsolete-in-example prompt line that the skill no longer collects, points skill references at the collapsed layout, and unstages generated files (index.xml, ns-*.xml, _filter.xml, FrameworksIndex/) that stub regeneration rewrites so they stay out of the PR. Validated on CI: two consecutive runs produced clean PRs (#168, #169) — dedup held, host gate 0 errors, agent ran format-first / source-first / three reviewers / no build, and MS Learn OpenPublishing reported Errors:0. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworks the
api-docsskill into a single dual-purpose tool that both adds docs for new APIs and reviews/corrects existing docs, in the standard agentskills.io layout, and replaces its PowerShell tooling with a Cake target.Structure (router + references)
The gh-aw CI sandbox does not honor per-sub-agent model routing, so the old multi-agent orchestration (writer + 3 reviewers + synthesizer, each with a
Model:header) was dead weight.SKILL.mdis now a thin router (~75 lines) that points at focused references; one agent runs the whole pass.Removed the
agents/,workflows/, andeval/custom dirs, the per-roleModel:headers, and the earlierreferences/scope-resolution.md+assets/scope-aliases.ymlselector machinery — natural-language scope is now resolved by the model. The prompt/methodology content is preserved, folded intoreferences/rather than deleted.Direct-XML editing
Drops the old extract→write→merge JSON round-trip (scaffolding for a weaker model). A capable model edits the mdoc XML in place; safety comes from
docs-format-docs, which formats every file and fails the build on broken XML/CDATA.Tooling: one Cake pass
Removes
scripts/docs-tool.ps1(−452) and folds format + content checks into a singledocs-format-docstarget inscripts/infra/docs/docs.cake(+213), wired in viabuild.cake. It loads each type XML once (XDocument), formats it, and runs the deterministic checks in the same pass — warnings for missing/placeholder/quality issues, build-failing errors only for broken XML/CDATA.Obsolete handling
Makes
references/obsolete-api-map.mdthe unambiguous, overload-aware authority and removes the name-only obsolete-in-example linter. Name matching could not tell an obsolete overload from a modern one (DrawText/MeasureTextexist on both the deprecatedSKPaintforms and the modernSKFont/SKCanvas(..,SKFont,..)forms), so it produced false positives and a skill that contradicted its own reference. The reviewer now judges obsolete use, guided by the map (the deprecated overload is the one without anSKFontparameter).Companion + validation
The
auto-api-docs-writeragentic workflow that runs this skill on CI is mono/SkiaSharp-API-docs#155 (single agent, Opus 4.7). Validated by two consecutive CI runs (PRs #168 and #169): consistent across both — 8 placeholders filled source-first, 23 lint findings fixed, 0 final findings, no obsolete usage introduced, no valid modernSKFont/SKCanvasoverloads deleted, and MS Learn OpenPublishing reported Errors:0.