docs(developing): define external integration boundary#8184
Conversation
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
Review: #8184 — docs(developing): define external integration boundary
Author: Audacity88 (Dan Gilles)
Head SHA: a4042a3
Verdict: --approve
Minimal, well-scoped docs-only PR. Adds the external-integration boundary decision table to first-party-extensions.md and threads a cross-reference into tools/overview.md. Content is accurate, CI is green, and the PR template is fully populated for a risk: low change.
✅ Scope — docs-only, no source leakage
Changed files: docs/book/src/developing/first-party-extensions.md (+21 lines) and docs/book/src/tools/overview.md (+4 lines). No labeler, workflow, Rust source, config schema, or generated file changes. Scope boundary claim holds.
✅ Accuracy — boundary table matches actual codebase surfaces
The new ## Choose Built-In Or External section documents six abstract surfaces (baseline built-in, feature-gated first-party, WASM plugin, skill package, MCP server, CLI-backed). None cite specific trait or file paths that could be wrong. Cross-checked against crates/zeroclaw-api/src/ — the six surfaces correspond correctly to the actual capability layers (tool.rs, model_provider.rs, channel.rs, memory_traits.rs, peripherals_traits.rs, WASM plugin ABI). No incorrect trait names, module paths, or outdated descriptions introduced.
✅ Links — all resolvable
[Skill package](../tools/skills.md)→docs/book/src/tools/skills.mdexists ✓../developing/first-party-extensions.md#choose-built-in-or-external— anchor is generated by the## Choose Built-In Or Externalheading added in this same commit; resolves correctly ✓- Existing links in the unchanged portions of both files are unmodified ✓
✅ Validation evidence — docs quality gate present
docs_quality_gate.sh output shown in the PR description with markdownlint-cli2 v0.20.0, 0 errors across both changed files. git diff --check clean. Evidence meets the docs-only bar.
✅ No AI/bot attribution trailers
Single commit a4042a3543cb docs(developing): define external integration boundary — no Co-authored-by: ...bot, Generated-by:, or AI attribution trailer.
✅ CI — all checks green
All 18 CI jobs pass, including Docs Style (25 s). No flaky or skipped jobs.
✅ PR template — complete for risk: low
Summary, Blast radius, Scope boundary, Linked issues, Validation Evidence (with literal command output), Security & Privacy Impact, Compatibility, and Rollback sections all present. Rollback section correctly notes this is low-risk and a revert suffices; no Rollback plan is required for risk: low per the review protocol.
🔵 Nit — docs_links_gate.sh "0 added links" can mislead future authors
The validation output reads Collected 0 added link(s)… No added links detected despite the new [Skill package](../tools/skills.md) relative link. The gate only scans for added http:///https:// external URLs — relative doc-to-doc links are outside its scope and are validated by the mdbook build (which CI runs). Not a defect in this PR, but a comment in the gate's output or its README would prevent future contributors from misreading a "0 links" result as "no new links of any kind." Consider filing a follow-up note or docs issue.
tidux
left a comment
There was a problem hiding this comment.
I read the PR end-to-end, verified the linked files and anchor resolution, ran the docs gates locally against the actual PR head, and checked the content against the existing IA of first-party-extensions.md. Stacking my approval on @WareWolf-MoonWall's at a4042a3. Substantive notes below: one correction to his gate-scope claim, one new finding on the PR body's docs_links_gate.sh evidence that doesn't reproduce, and the rest praise.
🟢 What looks good — section slots cleanly into existing IA
The new ## Choose Built-In Or External section at docs/book/src/developing/first-party-extensions.md:7-27 sits between the page intro (lines 1-5) and the existing ## Choose The Smallest Surface matrix (was line 7, now line 30). That ordering is the right call: the existing matrix answers "where does it go in the codebase?" assuming you've already decided to build it first-party; this PR adds the missing earlier question — "should it be built first-party at all, and at which trust/lifecycle tier?". Six abstract surfaces (baseline built-in, feature-gated, WASM plugin, skill, MCP server, CLI-backed) line up correctly with the actual capability layers in crates/zeroclaw-api/src/ (tool.rs, model_provider.rs, channel.rs, memory_traits.rs, peripherals_traits.rs, plus the plugin ABI) — verified by listing the trait files. No incorrect path or trait name introduced.
🟢 What looks good — links and anchor resolve
[Skill package](../tools/skills.md)fromdeveloping/first-party-extensions.md→docs/book/src/tools/skills.mdexists (file present, inSUMMARY.md:78)[first-party extension boundary](../developing/first-party-extensions.md#choose-built-in-or-external)fromtools/overview.md→ the anchor is generated by the H2 heading added in the same commit; mdbook slugification (lowercase + hyphenate) onChoose Built-In Or External→choose-built-in-or-externalmatches exactly- Both target files are wired into
SUMMARY.md(developing/first-party-extensions.md:108,tools/overview.md:75,tools/skills.md:78)
🟢 What looks good — framing honors #6165 without prejudging it
#6165 is the open RFC arguing for removing built-in tool code in favor of skills/MCP. This PR carves out the decision framework without removing anything, and explicitly states the boundary the RFC needs to clear: "An external integration is not, by itself, a reason to delete a first-party tool; the boundary depends on the contract ZeroClaw must preserve for operators, agents, and existing configs." That is the right doc-first move — anchor the criteria before anyone files a removal PR, then evaluate proposed removals against the anchor.
🟢 What looks good — em-dash gate respected
Zero — (U+2014) characters in the diff. New section uses commas, colons, and semicolons throughout. docs_quality_gate.sh's em-dash rule passes — verified in the actual job log at https://github.com/zeroclaw-labs/zeroclaw/actions/runs/27976437133/job/82795369010 ("No prose em-dashes in changed docs files.").
🟢 What looks good — single signed-off commit, no AI/bot trailers
a4042a3543 is the sole commit. No Co-authored-by: ...bot, no Generated-by:, no AI-attribution trailers. Title is conventional commits (docs(developing):). Commit is GPG-unsigned (%G? = N), which matches your existing pattern on this author identity (e.g. #8068 was also unsigned) — squash-merge neutralizes signing on merge.
🔵 Correction — docs_links_gate.sh actually does scan relative links, not just URLs
@WareWolf-MoonWall's 🔵 nit on the prior review reads "The gate only scans for added http:///https:// external URLs — relative doc-to-doc links are outside its scope". I read scripts/ci/collect_changed_links.py to check, and the gate does cover both:
URL_REat:14matcheshttps?://...and feeds them through directlyINLINE_LINK_REat:15matches every[...](target), andnormalize_link_targetat:67-102accepts relative paths, resolves them againstos.path.normpath(os.path.dirname(source_path) / target), and returns the resolved repo-relative path (e.g.docs/book/src/tools/skills.md)- Only
mailto:,tel:,javascript:, and pure-fragment (#...) targets are filtered out
So the gate covers both shapes. The argparse description string at :144 does say "Collect HTTP(S) links" which is the source of the impression that it's URL-only, but the implementation is broader. If anything, the cleanup follow-up is to fix the argparse description and add a comment about the resolved-relative-path handling — not to add relative-link coverage that already exists.
🟡 Warning — PR body's docs_links_gate.sh evidence doesn't reproduce against the real PR head
The Validation Evidence block claims:
Collected 0 added link(s) from 2 docs file(s). No added links detected in changed docs lines.
I ran the same script locally with the same arguments against pr-8184 properly checked out (BASE_SHA=$(git merge-base pr-8184 upstream/master) = 1a3fdb9b95) and the script reports:
Collected 2 added link(s) from 2 docs file(s).
…with docs/book/src/tools/skills.md and docs/book/src/developing/first-party-extensions.md (both targets of the two new relative links the PR adds) in the output file. So the PR body's "0 added links" output is probably from a stale local run where HEAD did not include the commit (e.g. the file edits were unstaged when the script ran, since added_lines_for_file uses git diff base HEAD --).
This is not blocking: (a) the content is correct, the links resolve, the ## Choose Built-In Or External anchor exists and matches the slug from the H2 heading; (b) the link gate is not currently wired into PR CI anyway (the Docs Style job at .github/workflows/... only runs docs_quality_gate.sh, not docs_links_gate.sh — confirmed in the live job log). But the Validation Evidence section is the contract that lets reviewers trust local runs without re-doing them, and this one doesn't reproduce. Worth a one-line re-run in a fixup commit (git commit --amend + force-push) so the body matches reality, or — equally fine — leave it and note in a reply that the local run was stale; either way the PR ships correctly. Demoted to 🟡 instead of 🔴 because the artifact (the actual links) is correct.
🔵 Suggestion — wire docs_links_gate.sh into Docs Style if it's meant to be a gate
Tangent to this PR, but the gate exists and works, and the only thing keeping silently-broken relative doc links out of master is the mdbook build that runs on push: master (in .github/workflows/docs-deploy.yml) — by which point the bad link is already merged. Wiring docs_links_gate.sh into the Docs Style job (or a dedicated Docs Links job) on PR would catch the failure mode this PR's evidence section was trying to demonstrate, in the right place. Out of scope here; worth filing as a separate docs-CI issue.
|
Thanks @tidux, I updated the PR body with a committed-head rerun. The earlier Re-running against the actual PR head collects the two expected relative links:
That also confirms the script does collect relative links. The earlier |
singlerider
left a comment
There was a problem hiding this comment.
I checked the new decision table, the anchor, and the cross-reference link.
🟢 What looks good: the boundary is stated as a contract, not a vibe
The framing that an external integration is not by itself a reason to delete a first-party tool, and that the decision turns on the contract ZeroClaw must preserve for operators, agents, and existing configs, is the right principle. The closing checklist (document the required contract first, do not remove tools until compatibility, config, security policy, operator visibility, and rollback are explicit) keeps this from becoming a license to rip out built-ins.
🟢 What looks good: the cross-link and anchor resolve
The tools/overview.md reference points at #choose-built-in-or-external, which is the exact anchor the new ## Choose Built-In Or External heading produces. The other referenced pages (skills.md, mcp.md, plugin-protocol.md, agents/overview.md) all exist, so there are no dangling links.
Docs-only, CI green, two existing approvals. The table's per-surface "avoid when" column is the part that earns its keep; it gives a reviewer a concrete test for misplacement rather than a generic preference.
Summary
mastertype: docs,risk: low,size: XS,docs,domain:architecture.Validation Evidence (required)
docs/book/src/tools/skills.mdanddocs/book/src/developing/first-party-extensions.md.Security & Privacy Impact (required)
No)No)No)No)Yes, describe the risk and mitigation: N/A.Compatibility (required)
Yes)No)NoorYesto either: N/A.Rollback (required for
risk: mediumandrisk: high)Low-risk PR: revert the docs commit if the boundary language needs to be replaced.
Supersede Attribution (required only when
Supersedes #is used)N/A.