Add kb-writer and kb-pr-open skills - #1358
Merged
Merged
Conversation
Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
Address code review: base the all-clean exit on branch state (git status/log vs dev) instead of run-local fix count, so already-committed content isn't stranded; use safe branch -d gated on no unmerged commits; drop personal machine path from the PR description memory reference. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
Address second code review round: - Fetch origin/dev before diffing and use the remote-tracking ref instead of a bare dev, which may not exist locally or may be stale; fail open to Step 5 if the git commands error instead of treating a failure as "clean." - Resolve kb-pr-open's internal self-contradiction on knowledge_article_id: the field's state is informational only, so drop the "add it if missing" and "must always be present" language that conflicted with the documented never-add/never-remove policy. - Drop citations to feedback_kb_* memory files that don't exist in the repo and are unreachable for other TSEs; the conventions they referenced are already inlined in the skill. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
Address third code review round: - Step 1 now strips a trailing "+ verbose" token before validating file paths, so the documented invocation form doesn't fail its own path check. - All-clean exit message now switches to dev before running git branch -d, since git refuses to delete the branch you have checked out. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
Address fourth code review round: - Move the stray ":::note is not the KB format" guidance out of the title:semantic Derek row (unrelated to titles, only read while scoring titles) into a new scan row (23) that actually enforces it: flag ::: admonitions and convert to blockquote callout form per kb_style_guide.md. Update row-count references from 22 to 23 accordingly. - All-clean exit now suggests git switch --detach origin/dev before git branch -d, since evaluating merge status against a stale or missing local dev could falsely refuse a delete. The shared-rule-drift finding (kb-pr-open vs kb-pr-review) is the same class of issue as the already-tracked dedup follow-up; evidence added as a comment on that issue instead of fixing here. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
Address fifth code review round: - Step 1 now checks Vale availability up front, and both this and the Step 2 fallback note render a missing/erroring Vale as "not run (Vale not installed)" rather than a silent false "Clean" row. - Ported the NetwrixKB scoping sentence explaining why KB files fire NetwrixKB.* rules only, and dropped BoilerplateCrossRef from the WeakLinkText fix guidance since that rule lives only in the Netwrix style and can never fire on docs/kb/ files. - Links check now recognizes pathname:///docs/... (the form rulebook §8 mandates for internal cross-doc links, in active use in 10 KB articles) in addition to /docs/..., and flags dotted version segments (8.2 instead of 8_2) per the same section. Restored the npm run build backstop note. - All-clean short-circuit pathspec widened from docs/kb/ to docs/, matching the blast radius of the title-change link-text sweep that runs immediately before it. The same BoilerplateCrossRef and pathname:// gaps exist in kb-pr-review (inherited, not introduced here); documented as follow-up evidence on the tracked dedup issue instead of fixing kb-pr-review in this PR. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
…e check Address sixth code review round: - kb_style_guide.md and derek/SKILL.md incorrectly allowed plural resolution headings (## Symptoms/## Causes/## Resolutions), contradicting kb-pr-open's and kb-pr-review's singular-only rule. Singular-only is the correct, established convention (confirmed by kb-pr-review); corrected the style guide and derek to match, and dropped the plural forms from kb-writer's Section Depth table. - All-clean short-circuit in kb-pr-open now checks the whole working tree and full branch history vs. origin/dev, not just docs/ — the exit message claims the tree is clean enough to delete the branch, which a docs/-scoped check couldn't actually establish (e.g. an uncommitted static/ asset would be invisible to the old pathspec). Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
Address seventh code review round: drop the stale "scope the pathspec to docs/" lead-in sentence, left over from an earlier revision, that contradicted the no-pathspec code block and whole-tree wording that already follow it. Reviewer's rulebook §12 circular-reference finding did not reproduce — §12 already contains the raw log-line/error-dump title rule content, not just a pointer back to the skill. Migration debt (169 files with plural resolution headings) and the cross-section table row-count drift vs. the rulebook are documented as follow-up evidence on the tracked dedup issue. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
When Vale is missing, Step 1 told the agent to jump straight to Step 3 (report), bypassing Step 2 (Dale and Derek) entirely. Step 3's N/N-scanned receipt mechanism would then certify checks that never ran, producing a false all-clean report on exactly the fresh-machine case (Vale not yet installed) this skill's TSE audience is most likely to hit. Point to Step 2 instead, which already has the correct continue-on-error guard for Vale. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
- Step 7 Option A now confirms with the TSE before running git
push, matching the skill's own stated rule ("does not run git
operations without explicit TSE approval") — the push was the
only ungated git action in the flow.
- "Update existing PR" no longer claims "I pushed" when Option B
means the TSE did the push, not the skill.
- Step 1's branch check now handles detached HEAD explicitly
(stop and warn) instead of falling through to "proceed," since
a later git push would have no upstream to resolve.
The Derek-rules-in-four-places finding is the same class of issue
as the tracked dedup follow-up; added as further evidence there.
The gh pr list fork-branch edge case is noted there too, flagged
as possibly out of scope rather than fixed.
Generated with AI
Co-Authored-By: Claude Code <ai@netwrix.com>
…t caveats - Create new PR Option A/B no longer tell the agent to push again — Step 7 item 1 already pushed (and now gates that push on TSE approval per the prior commit). Two push sites for one action was a leftover from before that gate existed. - Step 4 now skips the "Ready to apply fixes?" prompt when Step 3's report already shows zero Required fixes, going straight to the All-clean short-circuit instead. Kept the report-clean condition explicit rather than just reordering the git checks earlier — moving the short-circuit's git status/log checks alone in front of the prompt would have incorrectly short-circuited runs that found real, not-yet-fixed findings (nothing would be in git yet either way). - Added the same "recount if the table changes" caveat already on the Derek row to the kb-editing-conventions scan and cross-section consistency row counts, keeping the N/N scanned receipt honest as those tables grow. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
- Title-change link-text sweep grep pattern now makes pathname:// optional, so it catches pathname:///docs/... links (17 in the repo) instead of only the bare /docs/... form. - Links check no longer claims onBrokenLinks unconditionally throws — docusaurus.config.js relaxes onBrokenLinks/ onBrokenMarkdownLinks/onBrokenAnchors to warn for single-product builds (the faster path CLAUDE.md recommends), so the build backstop only reliably fails on a full build. - derek/SKILL.md now flags a pluralized resolution heading (## Symptoms/Causes/Resolutions) as a rename, not as a missing heading — matches the explicit heading-labels rule kb-pr-open and kb-pr-review already have, instead of relying on the presence check's generic "add this heading" message. - Reworded the "no git operations without approval" rule to carve out read-only status commands (fetch/status/log used internally by the all-clean short-circuit) from the approval gate that covers state-changing operations (commit, push, branch create/ delete) — the flow already ran the read-only commands unprompted; the stated rule now matches. - Disambiguated "no pathspec on either command" (which reads oddly against a 3-command block) to name status/log specifically. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
…d-HEAD note Proactive audit + latest review round, combined: - Restored "and represent sequential steps of the same procedure" to Gate B (scan row 8) — dropped relative to the rulebook and kb-pr-review, which would have applied Step N labeling to any 3+ subheadings regardless of whether they're actually sequential steps of one procedure. - Added the missing H1/sidebar_label-consistency bullet to the mechanical title-fix list — present in kb-pr-review, absent here, so the areas-table citation to this section pointed at content that wasn't there. - Repointed the ::: admonition rule's citation from kb_style_guide.md (which never mentions Docusaurus admonition syntax at all) to derek/SKILL.md §7, the actual source. - Links check and title-change sweep now also cover relative .md links (../other-article.md), which kb_style_guide.md's own "Do" example uses and which were previously excluded as "external." - All-clean exit message now tells the TSE to check out a branch before the next article, since the suggested branch-delete command intentionally leaves them on a detached HEAD, which this same skill's Step 1 guard would otherwise stop them on next run. Empirically verified (Vale installed locally) that NetwrixKB truly replaces rather than merges with Netwrix on docs/kb/ files, per a review round questioning that claim: a Netwrix-only rule (BoilerplateCrossRef) produced zero alerts on a docs/kb/ test file. kb-pr-open's existing guidance was already correct; no change needed there. kb-pr-review's contradicting guidance, plus its missing admonition scan row and stale link-sweep pattern, are already logged as follow-up evidence on the tracked dedup issue. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
…n, push -u
- Canonicalize the admonition-type list (note/tip/info/warning/danger/
important) across derek/SKILL.md §7 and kb-pr-open's scan rows 1
and 23 — the two files previously enumerated different four-item
subsets, neither covering :::info or :::danger (both in real use).
- Step 4's skip-the-prompt condition now keys on "zero findings of
any kind," not "zero Required fixes" — a file with only soft
reminders (semantic title reframes, knowledge_article_id notes,
low-priority keyword observations) was previously skipped past
the fix prompt entirely, giving the TSE no chance to act on them.
- All-clean exit message now has two variants: the original for
when the report was genuinely empty, and a new one for when
findings existed but were declined/dismissed — the old single
wording ("no fixes were needed") was false in the latter case.
- Step 7's numbered push step now carries the -u fallback that was
already in the STOP callout above it but not in the step actually
being executed, so a fresh branch with no upstream doesn't fail.
The Derek-rules-hardcoded finding and the un-anchored N/N receipt
risk are the same class of issue as the tracked dedup follow-up;
added as further evidence there, including the admonition-list
drift this commit fixes as a concrete example of the failure mode.
Generated with AI
Co-Authored-By: Claude Code <ai@netwrix.com>
…-ID state Per explicit product decisions on two open policy questions this review round surfaced: 1. Q&A title gerund exception: kb-pr-open's existing carve-out (Q&A titles stay in question/topic form, not forced to gerund) is now the stated policy everywhere. Fixed kb_style_guide.md and derek/SKILL.md §3, which previously required gerund form for ALL How-To titles with no exception — contradicting kb-pr-open, kb-pr-review (which already had the same carve-out), and derek's own article-type classifier (which uses "title starts with 'How to'/is a question" as the Q&A signal, so derek's classifier and title-format check were self-contradictory independent of this PR). 2. Callout severity taxonomy: collapsed to the two severities kb_style_guide.md already formally defines (NOTE, IMPORTANT). derek/SKILL.md §7 and kb-pr-open's scan rows 1/23 previously disagreed on whether WARNING/TIP were separate target severities (2 vs 3 vs 4-way splits), and none handled :::info/:::danger. Added an explicit six-to-two conversion mapping in derek §7 and kb-pr-open row 23, and removed WARNING from kb-editing- conventions.md §5's blank-line rule to match. Also added the missing "present but malformed" state to kb-pr-open's knowledge_article_id table — a value that doesn't start with kA+ alphanumeric (e.g. "12345") previously fell through to the never-rewrite policy meant for valid/placeholder/empty/missing states and was reported clean, while derek §1 already flags the same case as a Required fix. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
… gap 1. derek's article-type classifier determined type purely from title phrasing, contradicting this PR's own Q&A carve-out (Q&A titles are allowed to be non-interrogative). A Q&A article with a topical title fell through to Resolution (Symptom) and got flagged for missing headings it was never supposed to have — confirmed against real data: 131 of 234 ## Question files have non-interrogative, non-"How to" titles. Made the classifier structure-first (## Question+## Answer, ## Overview+ ## Instructions, ## Symptom/Cause/Resolution), falling back to title only when no recognized structure exists — matching how kb-pr-open/kb-pr-review already scope by structure, not title. 2. The "NOTE/IMPORTANT only" severity policy had no enforcement path for callouts already in blockquote form with a disallowed severity (> **TIP:**, > **WARNING:**, etc.) — every existing rule only matched Docusaurus ::: syntax. 16 files (15 TIP, 1 WARNING) were invisible to any check. derek §7 and kb-pr-open row 23 now flag both the ::: form and the wrong-severity blockquote form, with an explicit mapping covering both spellings of each severity. Migration blast radius for both changes logged on the tracked dedup issue rather than swept in this PR. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
…tus, report routing 1. derek's structure-first classifier (added last commit) only matched singular Symptom/Cause/Resolution headings. A plural- headed Resolution article (171 files use plural headings) fell through to the title fallback and got misclassified as How-To — exactly the failure mode the §2 plural-rename note exists to prevent. Now matches singular or plural for the structure check, and still flags plural as a rename per §2. Also switched the "/" separator between Symptom/Cause/Resolution to explicit "and" semantics via prose, since "/" read ambiguously as "any of" next to the Question/Answer and Overview/Instructions bullets, which correctly used "and". 2. kb-pr-open's images:location rule said PNG-only; 4 committed .webp images (docs/kb/endpointprotector, docs/kb/accessanalyzer) are real, referenced, non-violating images that would have been flagged. Now allows PNG or WebP. 3. The rulebook and kb-pr-open both called the Error: title prefix "retired," which contradicts kb_style_guide.md and derek/ SKILL.md §3, both of which still require it for Resolution (Error) titles, and 24 files actively use it. Corrected both to state the prefix is still required and distinct from the inline "Error - <phrase>" log-dump normalization, rather than claiming either is retired. 4. Step 3's report contract required a findings section for any non-clean Overview row, but only defined table shapes for Vale/ Dale/Derek — cross-section consistency findings had no place to be reported. Routed them into the Derek table with `cross-section` as the area, mirroring how kb-editing-conventions findings already route there. kb-pr-review's identical PNG-only wording noted as follow-up on the tracked dedup issue. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
…-dump precedence Per explicit decisions on two open questions: 1. derek's Error-vs-Symptom classification was circular: it classified Resolution as Error only if the title already started with Error:, then checked that Error titles start with Error: — trivially true by construction, so an Error article missing the prefix silently passed as Symptom. Now classifies by content (does the Symptom section name a specific, identifiable error code/message) independent of title, then checks the title against that classification as a real, reachable check. 2. The <Component> Error - <phrase> log-dump normalization now explicitly wins over and is exempt from the Error: prefix requirement, rather than leaving both rules asserted with no stated precedence. None of the three existing worked examples in the rulebook use the Error: prefix, so this keeps them valid instead of requiring a rewrite. Updated the rulebook §12, derek §3, and kb-pr-open's title-format rules to state the same precedence consistently. Also fixed two nits: escaped-metacharacter guidance for the title- change link-sweep regex (titles containing . or + would otherwise silently mismatch), and disambiguated "rows §7, §8" to "rows #7, #8" in the two example tables, since § means rulebook section everywhere else in the file. kb-pr-review's un-propagated callout-severity rule and the products.js hyphenation migration debt (explicit decision: keep the strict rule, treat as migration debt like the heading pluralization) are logged on the tracked dedup issue. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
The classifier section (Article Type Identification) said a Resolution (Error) title must start with Error: "regardless of what the title currently says," directly contradicting the §3 exception added in the same commit for titles already normalized to the <Component> Error - <phrase> log-dump form. An agent following the classifier section literally would flag the exact false positive that §3, kb-editing-conventions.md §12, and kb-pr-open's title-format rules were all just changed to prevent. Made the classifier defer to §3's actual condition instead of restating a stricter one. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
- Title-change link-sweep escape guidance said to escape ".", "+",
"(", ")", ":" — omitting "?" (which this PR's own Q&A carve-out
newly makes valid in titles, e.g. "How Do You Export Event Logs?"),
"*", "[", "]", "{", "}", "|", "^", "$", and incorrectly including
":" (not an ERE metacharacter). Replaced the partial enumeration
with "escape every ERE metacharacter" plus a grep -rF fallback
that sidesteps escaping entirely.
- kb-pr-review had fallen out of sync with three rule changes this
PR already made to derek/kb-pr-open: it still called the Error:
prefix "retired" (contradicting kb_style_guide.md, derek §3, and
kb-pr-open, and disagreeing with the reviewer on the same
article's title fix), its scan row 1 still listed WARNING as a
valid callout, and it had no equivalent to kb-pr-open's row 23
(flagging wrong-severity blockquotes and Docusaurus admonitions).
Given this has now recurred across four review rounds with
concrete file-level evidence each time, fixed directly here
instead of deferring again: updated the retired-prefix line,
added row 23, dropped WARNING from row 1, and bumped all five
"22 rows"/"22/22" references to 23. Also removed a stale "row 23
removed" comment that predated this reuse of row 23, and
disambiguated "rows §7, §8" to "rows #7, #8" to match kb-pr-open.
Generated with AI
Co-Authored-By: Claude Code <ai@netwrix.com>
… sweep 1. kb-pr-open's links row claimed "rulebook §8 mandates the pathname:// form for internal cross-doc links" — the rulebook only constrains pathname:// links that already exist (underscore version segments, must resolve), it doesn't mandate using that form. The bare /docs/... form is dominant (~218 KB files) and builds clean. Reworded to state all three internal link forms are valid and none should be converted to another. 2. The title-change link-text sweep in both kb-pr-open and kb-pr-review built a shell grep command by interpolating the old title into a single-quoted string. Over 20 KB titles contain an internal apostrophe (e.g. "Unable to Login to UI due to 'Invalid Token' Error"), which breaks out of a single-quoted shell string — on top of the regex-metacharacter escaping this already called for. Reworded to use a dedicated search tool with the title passed as a parameter (removing the shell-quoting layer entirely) while keeping the regex-escaping requirement, since the underlying match is still regex-based. Applied the same fix to kb-pr-review's copy, which had not been updated with the multi-form pattern from an earlier round either. 3. kb-pr-review's Overview-table bullet said "Cross-section consistency (all patterns)" while every other reference to this row in the same file says "6 patterns" — matched kb-pr-open's phrasing, including the recount caveat. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
…-labels scope 1. The Error:-prefix exemption only covered titles normalized from a raw log dump, but kb_style_guide.md's actual exemption is broader: any Resolution (Error) title where the error message itself already contains the word "error" (e.g. "Agents Have Become Unresponsive Error") is exempt, independent of whether it's a log-dump normalization. Dozens of real KB titles hit this broader case. Fixed in derek/SKILL.md (classifier and §3), kb-pr-open, kb-pr-review, and kb-editing-conventions.md §12 to state both exemptions consistently. Also fixed two derek/SKILL.md shorthand path references (should be .claude/skills/derek/SKILL.md from within .claude/references/). 2. products frontmatter rule had no valid target for three values in active use: general (44 files, cross-product content, no corresponding product.js entry), onesecure (5 files — has a real target, 1secure, so this one should still be flagged and corrected), and recoveryad (1 file — genuinely ambiguous, the copy script maps two different products to this KB folder). Carved out general/recoveryad as non-flaggable and clarified onesecure as a normal correction, in both kb-pr-open and kb-pr-review. 3. article-type: heading-labels was scoped to "Symptom Resolution articles" only, but the singular-heading rule applies to both Resolution formats (kb_style_guide.md, derek §2). An Error Resolution article with plural headings would have been skipped by this check and only caught downstream. Fixed in both kb-pr-open and kb-pr-review. Also added derek, kb-writer, kb-pr-open, and kb-pr-review to CLAUDE.md's Skills and Agents table — all four were missing. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
…solve Q&A wording contradiction 1. kb-pr-review/SKILL.md had no YAML frontmatter (no name/description/ argument-hint), unlike every other skill — including the two new ones this PR adds. Added it, matching the convention. 2. images:location still disagreed between kb-pr-open (PNG or WebP) and kb-pr-review (PNG only) — and neither actually matched the repo, which also has committed .jpg/.jpeg files. Widened both to "format doesn't matter, only location does," covering all four formats in active use. 3. :::caution (valid Docusaurus admonition syntax) was missing from every six-item ::: enumeration (derek §7, kb-pr-open/kb-pr-review rows 1 and 23) despite the blockquote half of the same rule already targeting > **CAUTION:**. Added it everywhere. 4. Derek's structure-first classifier required ALL of a type's headings before classifying (## Question AND ## Answer, etc.). An article missing just one heading fell through to the title fallback and got misclassified as an unrelated type, demanding a Symptom/Cause/Resolution structure it never needed. Changed to partial-match tolerance (any one of a type's headings is enough to classify; §2 flags the rest as missing) — extending the same forgiveness already given to Resolution's plural-heading case. 5. kb_style_guide.md's Q&A title rule was self-contradictory: "describes the topic, not the action" immediately followed by "a 'How to...' title is expected" (which is action-form). Fixed in kb_style_guide.md, derek §3, and the gerund-carve-out wording in both kb-pr-open and kb-pr-review to state consistently that topic-descriptive, "How to...", and question-form titles are all valid for Q&A — none is a gerund violation. Also caught while reviewing: kb-pr-review's links row was still the pre-sync version (missing pathname:// and relative-.md link forms, and the unconditional build-backstop claim) — synced to match kb-pr-open's already-fixed version. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
…LAUDE.md 1. The partial-match classifier fix from the previous commit checked Q&A and Instructions before Resolution, with "first match wins." Since a Resolution article can legitimately carry an ## Overview heading alongside Symptom/Cause/Resolution, that combination now matched the Instructions check first and misclassified. Confirmed against three real KB files: two Overview+Symptom+Cause+Resolution articles would have been told to add an unneeded ## Instructions section and gerund-ize an error title; a Symptom+Cause+Answer article would have been told to add ## Question instead of being told its real defect (## Answer should be ## Resolution) — the exact opposite of what the prior title-based rule got right. Reordered to check Resolution first, since How-To articles never carry ## Symptom but Resolution articles routinely carry ## Overview — the asymmetry that made this safe to reorder. 2. docs/CLAUDE.md (auto-loaded whenever an agent works under docs/) still asserted KB images are PNG-only, contradicting the format rule this PR already relaxed in kb-pr-open and kb-pr-review to match what's actually committed on disk (PNG/WebP/JPG/JPEG). Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com>
Contributor
Code ReviewScope check first: this PR changes only agent-instruction and guidance files — two new skills ( Since the substance of the change is instructions that agents will act on, I verified the concrete repo claims they assert. They all hold:
I also stress-tested the one piece of new logic here — the structure-first classification order in Derek. Against the real corpus, the four articles that carry both a Symptom/Cause/Resolution heading and an Adding YAML frontmatter to One minor inconsistency
Two heads-ups on blast radius (intentional, not defects)
No correctness, security, config, script, or workflow issues found. |
hilram7
added a commit
that referenced
this pull request
Aug 12, 2026
…ions.md (#1370) * Dedup KB skill rulebook: extract shared rules into kb-editing-conventions.md derek, kb-pr-open, and kb-pr-review each independently inlined Derek's areas table, title-format rules, and the callout-severity mapping, requiring N synchronized manual edits per rule change (root cause of most add-kb-skills PR #1358 review rounds). Extend the existing kb-editing-conventions.md sections (callouts, bolding, links, titles) and add new ones (frontmatter, article-type identification, article structure, images) as the single source; all three skills now read from it instead of duplicating. Also reconciles the knowledge_article_id policy (kb-pr-review allowed proposing removal of a placeholder value; kb-pr-open never did) to kb-pr-open's never-remove policy, fixes kb-pr-review's stale BoilerplateCrossRef note (the Vale rule is unreachable on docs/kb/ files), and syncs wording drift in the kb-editing-conventions scan table and cross-section consistency table between the two PR skills. Addresses netwrix-corp/docs-kb-staging#131. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address code review: fix build-warn claim, restore dropped title/style-guide rules - docusaurus.config.js only relaxes onBrokenLinks under DOCS_PRODUCT; onBrokenMarkdownLinks and onBrokenAnchors throw unconditionally. Rulebook §8 overstated this as "relaxes all three to warn". - §12 Titles referenced Error: prefix exemptions without ever stating the base Error: prefix rule, and dropped the Resolution (Symptom) title-format rule entirely during extraction. Added a "Base title format by article type" subsection covering all four types. - Restored the kb_style_guide.md read instruction in kb-pr-open and kb-pr-review — both still depend on it (product-names, images alt-text) but dropped the explicit read step when switching to the rulebook as primary source. - Derek's knowledge_article_id missing-field soft reminder (inherited from kb-pr-open's already-established policy via the reconciliation) had no note text in Derek's Output section. Added it alongside the existing blank-value and placeholder note text. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address second code review round - Restored the dropped "multi-line commands/command output use fenced code blocks" rule (rulebook §10); repointed kb-pr-open/kb-pr-review's formatting: lists area at it alongside §3. - Derek now inherits a title-case check via §12 that overlaps Vale's NetwrixKB.HeadingCase (scope: heading, fires on the H1) - noted in §12 that this is Vale's finding, not a second Derek row, for any skill running both in the same report. - Qualified §12's "must not contain a product name" line - it's a general rule, not absolute; the semantic-reframes subsection below it is what actually governs (soft reminder, never auto-applied). - Removed Derek's standalone Keywords/Description section, which had already drifted from §13's carve-out for plausible-but-unused keywords; §13 Frontmatter already covers both fields. - Noted in Derek's Links area that WeakLinkText/BoilerplateCrossRef in §8 are Vale-only and not Derek's job. - Added a rulebook-vs-kb_style_guide.md precedence statement: the rulebook is a derived elaboration, not a replacement - divergence from the published style guide is a bug in the rulebook to fix, not grounds for the rulebook to win. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address third code review round - Dropped the Links and Images rows the extraction had silently added to Derek's areas table. Derek's Overview and description frontmatter never claimed those areas, and its single-file "read from disk" flow doesn't fit link-target/image-location resolution across the repo - that's kb-pr-open's and kb-pr-review's job. Restored Derek to its original five-area scope; added an explicit note pointing links/ images checks at the other two skills. - §13's title/H1-match rule now covers the no-H1 case (13 KB files have none) - Docusaurus renders frontmatter title as the H1 when the body has none, so that's a valid state, not a mismatch. Updated Derek's line-number guidance to fall back to line 1 when there's no H1 to point at. - §8's build-backstop note now says pathname:// links have no build backstop at all (full or single-product) - pathname:// is Docusaurus's documented escape hatch that bypasses the broken-link checker entirely, unlike /docs/... links which are at least caught by onBrokenMarkdownLinks/onBrokenAnchors on a full build. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address fourth code review round: dedup the scan tables, not just sync them The 23-row scan table and 6-row cross-section table were still fully duplicated verbatim between kb-pr-open and kb-pr-review, and now tripled a few rules against the rulebook (§5's 4-space indent vs. scan row 1, §10's table-cell periods vs. scan row 5) - the exact whack-a-mole pattern this PR exists to close out, just one level deeper than the areas table this PR already fixed. Condensed both tables' "what to flag/fix" column to the pattern description plus a rulebook-section pointer, dropping the duplicated rule-mechanics prose while keeping every kb-writer routing decision (rows 13/17/18/20) that isn't in the rulebook. Row count is unchanged (23 scan rows, 6 cross-section rows) so the N/N-scanned coverage mechanism is untouched. Verified the tables are still byte-identical between the two skills afterward. Also names explicitly that the cross-section table's rows 1-2 are the two failure directions of one rulebook §11 bullet, not two separate rules - the exact 6-vs-5 drift example from issue #131's history. Also: removed kb-pr-open's now-fully-redundant knowledge_article_id/ title-format pointer paragraph (duplicated the areas table's §12/§13 rows, sitting in a dead spot after the all-clean short-circuit), and added keyword/description quality to Derek's frontmatter area parenthetical so its scope description matches the PR skills'. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address fifth code review round: two regressions, one latent gap, one legibility nit - derek/SKILL.md's scope sentence (added by this PR) contradicted its own areas table two lines above, explicitly excluding callouts (§5) and bolding/path formatting (§6) that the table sends Derek to. A literal regression: those were unambiguously in scope pre-PR as Derek's own §5 Path Formatting and §7 Admonition Format sections. Fixed the Overview line, frontmatter description, and scope sentence to all state the same six-area scope. - §13's keywords row lost the "flag terms prominent in the article body that are missing from keywords" check during extraction - both source files had it, the consolidated row only kept the inverse direction (keyword present but unused in body). Since this drives KB search discoverability, restored it as a Required fix distinct from the softer inverse-direction observation. - §12's Vale-deferral for title case fires unconditionally, but Vale's NetwrixKB.HeadingCase is heading-scoped and never evaluates the 13 KB files with no body H1 (where Docusaurus renders frontmatter title as the H1 instead). Scoped the deferral to H1-bearing articles only; no-H1 articles now get the frontmatter title's case checked directly since nothing else will. - Promoted the Error: prefix's two exemptions to a top-level sub-bullet next to the Resolution (Error) rule they modify, instead of nesting exemption (a) - a general rule unrelated to log-dump normalization - under the narrower "raw log dump" bullet where a model evaluating that bullet as inapplicable would never reach it. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address sixth code review round - Carved out §12's product-name-in-title and §13's no-H1-is-valid rules from the new rulebook-vs-style-guide precedence statement - both read as literal disagreements with kb_style_guide.md's absolute wording, but are deliberate elaborations, not bugs to revert. - Fixed derek/SKILL.md's area count: Product Names lives in its own section below the table, not as a table row, so "six-area scope" underclaimed by one against the Overview's seven-area list. - Restored the no-target-found branch for <!-- link removed --> (leave the comment, note unresolved) - dropped in the scan-table condensing pass, in both skills and the rulebook. - Scan row 4 (intra-file bolding consistency) pointed at §6, but §6 had no consistency rule to back it - added one, closing the exact kind of dangling-pointer gap this PR exists to prevent. - Moved the products.js-vs-style-guide-table precedence into §13 once; kb-pr-review's product-names row carried it inline while kb-pr-open's didn't - now both rows are identical and inherit it from the rulebook. - Nit: clarified the Error: prefix exemption (b) is actually a special case of (a), not an independent condition - it's named separately because it's the specific pattern the log-dump rule produces. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address seventh code review round - Derek's output contract had no channel for the soft/judgment findings it now inherits from §12's semantic title reframes (product-name-in-title, article-type/title mismatch) - previously a counted hard-check row, now correctly a non-auto-apply soft reminder, but Derek's "every issue is a table row" contract gave it nowhere to go. Added a fourth soft-reminder note type alongside the three knowledge_article_id states. - Clarified the naming collision between "Derek" as a findings-table label (used by kb-pr-open/kb-pr-review for their own inline links/images checks) and the /derek skill (which doesn't check links/images at all as of this PR). Added a one-line naming note to both PR skills. - Verified §14's "confirmed against real KB files" claim for the Resolution-first classification order — it holds: 3 real articles (excluding the known multi-template file) have Overview alongside Symptom/Cause/Resolution, so the rationale text is accurate and wasn't changed. Documented the mirror-direction edge case (Q&A article with a stray Overview heading) that does affect exactly the one template file, so it isn't mistaken for a new bug later. - CLAUDE.md's skills table still described /derek's pre-PR scope; updated to include callouts and bolding/path formatting. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address eighth code review round - derek/SKILL.md's closing line ("never respond with anything beyond the assessment line and output table") forbade the four soft- reminder note types the Output section just above it defines, including the new semantic-title-reframe note. Under a strict reading, product-name-in-title and article-type/title-mismatch could vanish from Derek's output entirely. Amended to explicitly allow the defined soft reminders. - §12's title-case Vale-deferral read as an unconditional instruction, but it only resolves the duplicate-finding problem for skills that run Vale in the same report. A standalone /derek invocation doesn't run Vale, so on an H1-bearing article a mis-cased title still goes unchecked - not a regression (Derek's own Vale-handles list already excluded heading case), but the new text implied a deferral that doesn't exist for that invocation path. Made the condition explicit. - Nit: clarified derek/SKILL.md's "seven-area scope" claim is a total count, not a row-by-row correspondence with the Overview line's list (which groups keyword/description under frontmatter and splits article type/structure differently than the table's §14/§15 rows). Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address ninth code review round - §12's title-case bullet inverted its own premise: it said Derek's "what Vale already handles" list "excludes heading case," when the list actually assigns heading case to Vale (that's why a /derek-only run misses it). As written, an agent resolving the contradiction literally would start flagging H1 title case in every invocation, duplicating Vale. Fixed the wording to state the premise correctly. - The "Naming note" added to both PR skills undercounted the /derek scope delta - it named only links/images, but formatting: lists (§3+§10) and prose-directness (§4) are also Derek-table areas that /derek itself doesn't check. Both skills now name all four. - Restored WARN to the raw-log-dump title trigger's level-token list - both deleted source copies had it, the consolidated §12 dropped it incidentally during extraction. - Added a parenthetical to both skills' "do NOT flag heading case" line clarifying it means H1 case only - the §12 no-H1 carve-out (check the frontmatter title directly) isn't covered by Vale and still applies, so the two lines don't actually conflict. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address tenth code review round - §13's product-ID carve-out header said "three values are exceptions to the strict-match rule" but the first bullet (onesecure) said to flag and correct it - contradicting both the header and its own sibling bullets' "never flag." Restructured so only general/ recoveryad are labeled carve-outs; onesecure is explicitly "always flag." - Scan row 8's "repetition-signaled clusters" claim had no home left in §1 Gate A after extraction - the base rule's thin "Remove X"- cluster merge criterion never made it into the consolidated section. Restored it as Gate A's second merge criterion. - The precedence preamble claimed exactly two deliberate rulebook-vs- style-guide elaborations; two more exist in the extracted text (sidebar_label truncation as a mechanical Required fix vs. the style guide's softer "may be shortened," and the knowledge_article_id presence-is-informational policy vs. the style guide's unqualified Required-field listing). Added both to the list. - Synced kb-pr-review's scan-row label wording to match kb-pr-open's exactly (both now say "as of this writing; recount if..."). - Moved the "don't flag > **NOTE:** blockquotes as :::note admonitions" line out of kb-pr-review's title:semantic row (where it has nothing to do with titles) into rulebook §5 Callout severity, its actual home; kb-pr-open never had the stray copy. - Added a one-line carve-out to the rulebook preamble noting Derek doesn't apply §8/§16, so "apply during the derek review" doesn't read as covering areas Derek's own Review Areas table excludes. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address eleventh code review round - Rulebook preamble's Derek-scope carve-out named only two excluded sections (§8, §16) as if that were the complete exclusion list, when Derek's Review Areas table actually includes just six sections total (§5, §6, §12-§15). Restated as an inclusion list - the same fix already applied to both PR skills' "Naming note" - so the preamble isn't a third, less accurate copy of the same scope statement. - The heading-case Vale-deferral parenthetical (added last round to both PR skills) never reached derek/SKILL.md itself. Derek's own "what Vale already handles" list still said heading case unqualified, which would make an agent resolve the no-H1 exception against its own closer, more specific instruction and skip the finding §12 exists to catch. Added the same parenthetical to Derek's list entry. - Added an explicit "Derek reports; it never edits" guard. The rulebook sections Derek now reads are written in fixer voice for the applying skills ("convert/relabel," "must be wrapped in backticks") with no per-skill framing to signal report-only intent anymore. - Nits: added line-number guidance for Derek's two new areas (§5, §6); consolidated the sidebar_label truncation rule to one copy in §13, with §12 pointing to it instead of restating it. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address twelfth code review round: sync title/H1/sidebar_label on any one fix §13 requires frontmatter title, H1, and sidebar_label to agree, but nothing said to keep them in sync when correcting just one — including a Vale HeadingCase fix that rewrites only the H1 line. That fix would otherwise introduce a fresh §13 violation immediately after Derek's pass reported clean, with nothing in the same run left to catch it. Added an explicit "fix all three together" instruction to §12's mechanical-fixes list; both kb-pr-open and kb-pr-review inherit it automatically since they already route title:mechanical to §12. The other item from this round (Derek's frontmatter/title scope correctly growing louder on ~300 legacy hyphenated-product-ID files) is a real, intended behavior change already documented as accepted migration debt in the rulebook preamble - not a defect, no fix needed. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address thirteenth code review round - Derek's output contract had no channel for §13's low-priority keyword observation (a keyword absent from the body but a plausible search term) - explicitly not a Required fix per §13's own text, but also not one of Derek's two defined soft-reminder types. Added it as a third soft-reminder bullet, matching the severity kb-pr-open already handles correctly for the same rule. - Nit: Derek's links/images exclusion rationale ("outside Derek's single-file review model") didn't actually distinguish those checks from two areas Derek does own - the §13 products.js lookup and the Product Names "verify against product docs" check are also outside the article file. Rewrote the rationale around what actually differs: link/image resolution is an unbounded search across docs/kb/**, while products.js and product-docs checks are one fixed lookup against one known source. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address fourteenth code review round - §14's Q&A+Overview edge-case note pointed at docs/kb/accessanalyzer-2601/kb-article-template.md as the one file that hits it - verified that file actually has Symptom/Cause/ Resolution headings too, so under the Resolution-first rule it classifies as Resolution, not the Q&A/Overview collision the note describes. Verified corpus-wide the edge case hits zero files (every ## Question+## Overview file also has a Symptom/Cause/ Resolution heading). Rewrote the note to state that accurately instead of citing a file that demonstrates the opposite. - "Heading case ... H1 case only" (derek) and the matching parenthetical in both PR skills understated NetwrixKB.HeadingCase's actual scope - it's scope: heading, firing on every heading level, not just the H1. As written, the do-not-re-flag list read as license to flag H2-H6 case as a new finding, duplicating Vale. The real gap is narrower: an article with no body H1 at all, where Vale's heading-scoped rule never runs on anything. Fixed all three copies to state the correct scope and the correct (narrower) gap. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address fifteenth code review round: keep authoring notes out of the report Both skills' Overview-table row-label instructions baked "as of this writing; recount if the table changes" directly into the literal string used as the row label - that maintenance note would render verbatim in the posted PR review comment. Moved it to prose next to the instruction, kept the row labels short (matching the example output blocks, which already used the short form). Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address sixteenth code review round - §16's exclusion from Derek was justified by an "unbounded filesystem lookup" rationale that only actually applies to the location rule. external-refs (is this URL an external CDN link) and alt-text (does alt text just repeat the filename) are pure single-file text checks, same cost class as everything else in Derek's scope - dropping them had no real justification. Moved both into Derek's scope; kept location with kb-pr-open/kb-pr-review since it genuinely requires checking the filesystem beyond the article. Updated Derek's areas table, Overview line, description frontmatter, line-number guidance, and the rulebook preamble's inclusion list to match. This also fixed the "four areas" nit in both PR skills' naming notes for free - the excluded set is now exactly four rows (links, images: location, formatting: lists, prose-directness), matching the stated count. - The precedence preamble's "four deliberate deviations" list missed two real ones: §6's action-target-only bolding rule (contradicts the style guide's unconditional "bold for UI elements...") and §13's products.js-authoritative rule (the style guide's own example frontmatter uses a non-canonical underscored value). Added both so a future reader doesn't "fix" either back to the literal reading. - PR description now lists four /derek behavior changes instead of two: the previously-undocumented product-name-in-title downgrade (counted finding -> uncounted note) and the newly-in-scope callout- indent/bolding-consistency rules, plus a note on the images scope split. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address seventeenth code review round - derek/SKILL.md's Derek-vs-Overview reconciliation note was stale against last round's Overview rewrite: pointed "below" at a line that's actually above it, and described the Overview as folding keyword/description into frontmatter and not itemizing images - neither true anymore, since the Overview now lists all eight areas explicitly. Simplified to state the now-exact match (8 table+Product Names areas = 8 Overview items) instead of reconciling against wording that no longer exists. - The "authoring notes must not appear in the report" cleanup two rounds ago covered the scan-table and cross-section row labels but missed the Derek (N checks) bullet's own "21 as of this writing; recount if..." clause in the same list, in both PR skills - the exact kind of partial fix this PR exists to close out. Stripped it and extended the standalone authoring note to cover all three counts (21/23/6) instead of just two. - §16's "do not flag file format" guard sat inside the Location bullet - the one row Derek is explicitly told to skip - while Derek now reviews the other two rows (external-refs, alt-text), one of which (alt-text) uses a .png example. Promoted the guard to a lead-in covering all three rows regardless of which skill reads which row. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address eighteenth code review round - §12's title-case Vale deferral was keyed on which skill is running, not on whether Vale actually produced output for this file. Both PR skills explicitly continue when Vale is missing or errors (kb-pr-open: "Vale | not run", kb-pr-review: "Vale | Not available - skipped"), so on a machine without Vale the deferral silently dropped the check entirely - a real regression versus base derek/kb-pr-open/kb-pr-review, which all checked title case unconditionally with no deferral. Fixed to key the deferral on Vale having actually run and reported, with an explicit fallback to checking the H1 directly when it didn't. Synced into both PR skills' parenthetical clarifications. - derek/SKILL.md's "eight areas = eight Overview items" arithmetic check was coincidental, not a real correspondence - the two lists partition the same scope differently (Overview collapses article type/structure into one item; the table splits it into two rows). The match would silently stop meaning anything the next time either list changes. Dropped the count assertion rather than keep an invariant that isn't actually one. - kb-pr-review lacked kb-pr-open's findings-routing rule (scan-table and cross-section findings route into the Derek table with kb-editing-conventions/cross-section as the area, not their own section) - a pre-existing gap, cheap to close now that both tables are otherwise synced. - PR description now lists the §15/§13 checks (Q&A question-format, How-To structure, wrong-shape, title/H1 exact-match) that grew standalone /derek's scope beyond what was previously called out. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> * Address nineteenth code review round: fix genuine self-contradiction §12's title-case bullet told /derek two incompatible things in the same paragraph - a bold clause saying /derek should check H1 title case directly (no deferral to make for a standalone invocation), and a parenthetical immediately after saying nothing changes for /derek because it already defers heading case to Vale. derek/SKILL.md:13 sided with the parenthetical. Since /derek never runs Vale, a strict reading of the bold clause would have made every H1-bearing article with bad title case a new Required-fix row - a mass finding class introduced by my own fix two rounds ago, and not mentioned in the PR description's behavior-change list because it wasn't intended. Resolved by scoping the whole bullet to kb-pr-open/kb-pr-review (the skills that actually run Vale) and removing /derek from its "no deferral" clause entirely, leaving derek/SKILL.md:13's existing Vale-handles-heading-case-except-no-H1 policy as the sole authority for what /derek checks - no new mass-finding class, no contradiction. Also: disambiguated "the canonical list in src/config/products.js" to name the products array specifically, since the file also has a categories array whose ids do use hyphens (directory-management, endpoint-management, identity-management) - zero live impact today (no KB file uses those values) but removes a live ambiguity. Generated with AI Co-Authored-By: Claude Code <ai@netwrix.com> --------- Co-authored-by: Claude Code <ai@netwrix.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.
Adds Claude skills for creating KB pull requests