Skip to content

feat: add doc-accuracy reviewer to catch harmful documentation changes - #714

Open
Iheanacho-ai wants to merge 1 commit into
siderolabs:mainfrom
Iheanacho-ai:check-doc-code
Open

feat: add doc-accuracy reviewer to catch harmful documentation changes#714
Iheanacho-ai wants to merge 1 commit into
siderolabs:mainfrom
Iheanacho-ai:check-doc-code

Conversation

@Iheanacho-ai

@Iheanacho-ai Iheanacho-ai commented Aug 20, 2026

Copy link
Copy Markdown
Member

What

Adds make check-doc-accuracy, a reviewer that checks docs for changes that could harm a reader who follows them (commands that lose data, destroy things, or weaken security, not just typos). Also runs it in CI on every docs PR, forks included, as an advisory check.

Why

The worst doc bugs are commands that run fine but still do damage, like a service started without its data mount or a dropped safeguard. They pass every syntax check, so a flag checker won't catch them. You need something that reads the snippet and reasons about what happens if you run it. Until now that only ran when someone remembered to run it locally.

Change

  • tools/doc-accuracy: a Go program that drives the claude CLI as a read-only reviewer (it reports, never edits). It reviews the .mdx files you changed and checks claims about Talos/Omni against the upstream source.
  • Looks for harm first: data loss, destructive commands, removed safeguards, security downgrades, then plain wrong flags and values.
  • Output: a short terminal summary, a JSON file (verdict + a reason and fix per finding), and in CI, inline annotations plus one summary comment that updates in place on each push.
  • CI (.github/workflows/doc-accuracy.yaml): uses pull_request_target so it works on fork PRs. Checks out our base branch only, never the fork's code, and reads the PR as a text diff. Nothing from the PR is run. It never blocks merge.

Usage

make check-doc-accuracy                                # files you changed vs HEAD
make check-doc-accuracy DOC=public/omni/....mdx        # one specific file
make check-doc-accuracy DOC_ACCURACY_BASE=origin/main  # PR-style, vs a branch
make check-doc-accuracy-all                            # the whole public/ tree (slow)

Testing

  • go test, go vet, and gofmt all pass.
  • Ran it against a sample doc with four planted problems (etcd with no volume mount, rm -rf /, --insecure on a bootstrapped node, a stale image tag). It caught all four at the right severity and checked the current Talos version upstream. Verdict FAIL, exit 1.
  • Confirmed only a CRITICAL fails the run; warnings and notices don't. No changed docs pass cleanly, and a missing claude CLI gives a clear message.
  • Not yet tested on a live PR, since pull_request_target runs the workflow from main, so the annotations and comment only show up once this merges. The ANTHROPIC_API_KEY secret is set.

@github-project-automation github-project-automation Bot moved this to To Do in Planning Aug 20, 2026
@talos-bot talos-bot moved this from To Do to In Review in Planning Aug 20, 2026
@smira smira removed this from Planning Aug 20, 2026
Comment thread tools/doc-accuracy/main.go Outdated
args = append(args,
"--output-format", "stream-json", "--verbose",
"--permission-mode", "bypassPermissions",
"--allowedTools", "Read", "Grep", "Glob", "WebFetch",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Claude:
--allowedTools is ignored under --permission-mode bypassPermissions (claude-code#12232). The whitelist here isn't actually a whitelist, so Bash is unrestricted and the read-only claim doesn't hold. --disallowedTools Edit,Write should still block those two. --permission-mode dontAsk with this allowedTools list, or --tools as a real whitelist, would do what this is aiming for.

that previously made the procedure safe. Compare against the diff and the
upstream source; a deletion that makes a command *more* dangerous is a
top-priority finding even if what remains is valid.
- **Security downgrades / exposure** — disabling TLS or auth, `--insecure`,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--insecure as CRITICAL will fail half the Talos bootstrap corpus (talosctl apply-config --insecure). Worth carving out the documented bootstrap case so this doesn't train us to ignore the tool.

Comment thread Makefile Outdated
DOC_ACCURACY_MODEL ?=

.PHONY: check-doc-accuracy
check-doc-accuracy: ## AI-review docs for accuracy/harm. Scope one file with DOC=public/path; base with DOC_ACCURACY_BASE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default base is HEAD, so after a local commit this won't actually review anything (DOC_ACCURACY_BASE=origin/main is in the README but easy to miss). Defaulting to origin/main (with a fallback) would match how this will actually get run.

@Iheanacho-ai
Iheanacho-ai force-pushed the check-doc-code branch 2 times, most recently from bbf5052 to 1ad23e7 Compare August 28, 2026 15:19
@Iheanacho-ai
Iheanacho-ai marked this pull request as draft August 29, 2026 08:43
@Iheanacho-ai
Iheanacho-ai force-pushed the check-doc-code branch 3 times, most recently from 58c0880 to 8b109db Compare August 29, 2026 12:19
@Iheanacho-ai
Iheanacho-ai marked this pull request as ready for review August 29, 2026 12:30
@talos-bot talos-bot moved this to In Review in Planning Aug 29, 2026
@smira smira removed this from Planning Aug 31, 2026
Comment thread .github/workflows/doc-accuracy.yaml Outdated
set -euo pipefail
MARKER='<!-- doc-accuracy-comment -->'
FINDINGS="_out/doc-accuracy-findings.json"
# If the review step never produced a file (e.g. it errored early),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the review step errors (timeout, claude crash, no verdict), this manufactures a PASS. Combined with the "existing comment" branch below, a failed run will overwrite a previous FAIL with "Nothing flagged."

Gate this on steps.review.outcome. If the review step didn't succeed, post "reviewer didn't finish, see the log" and leave the last real comment alone. A missing findings file is not a clean review.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed. The tool now emits a tri-state verdict (PASS/FAIL/INCOMPLETE); a crash or no-verdict run is INCOMPLETE and posts a separate 'didn't finish' note instead of overwriting the findings comment

Signed-off-by: Amarachi Iheanacho <amarachi.iheanacho@siderolabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants