Skip to content

Update audit-fix skill for new docs-audit process - #1483

Open
jth-nw wants to merge 2 commits into
devfrom
feat/audit-fix-skill
Open

Update audit-fix skill for new docs-audit process#1483
jth-nw wants to merge 2 commits into
devfrom
feat/audit-fix-skill

Conversation

@jth-nw

@jth-nw jth-nw commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Exclude Identity Manager 7.0 and PingCastle 4.0 from the generated docs-audit list
  • Update the audit-fix skill and docs-audit/template.md to match the new docs-audit process: drop the Duplicated in/Accurate/Complete/Fix Summary workbook fields, standardize branch naming to doc-audit/{product}/{detail}, move the fix summary into the commit message, and add fork-based fan-out for batches of documents pasted in one go
  • Corresponding Xchange "Docs Audit Process" doc updated to match

Test plan

  • Paste a single-document correction through the audit-fix skill and confirm branch naming, sibling detection, and commit message follow the new format
  • Paste a multi-document batch and confirm fork fan-out and question batching work as described

jth-nw and others added 2 commits September 4, 2026 12:50
Both versions are out of scope per the Netwrix-Docs-Audit.xlsx dashboard.
Adds them to AUDIT_EXCLUDED_VERSIONS and regenerates identitymanager's
review-list.csv accordingly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drop the Duplicated in field and Accurate/Complete/Fix Summary workbook
columns that no longer exist, standardize branch naming to
doc-audit/{product}/{detail}, move the fix summary into the commit
message, and add a fork-based fan-out for batches of documents pasted
in one go.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Code Review

Scope reviewed: scripts/generate-audit-list.mjs, docs-audit/identitymanager/review-list.csv, docs-audit/template.md, .claude/skills/audit-fix/SKILL.md. No Docusaurus config, sidebar, KB-pipeline, or workflow files are touched, and there are no security concerns (no secrets, no shell interpolation, no new execution paths).

The CSV regeneration checks out cleanly: 46 7.0 rows removed (exactly the 696 − 650 line delta), current rows unchanged at 698 in both base and head, and zero dangling 7.0 references left in the duplicates column. The version exclusion is applied before duplicate detection in generate-audit-list.mjs:412, so no orphaned primary rows are possible.

Four issues, roughly in severity order.

1. Fork fan-out inverts the skill's own "don't lint before approval" gate

.claude/skills/audit-fix/SKILL.md, "Handling multiple pages in one paste"

Step 3 of the fan-out assigns each fork "Steps 2, 3, 5, 6, and 7." But Step 3 (confirm scope) and Step 6 (show diff, wait for confirmation) both require reviewer interaction, and Step 7 is explicitly gated: "Once Vale is available and the reviewer has confirmed the diff…". A fork can't reach the reviewer — the same paragraph tells it to stop and report questions back to the parent instead.

That leaves two contradictory readings, and neither works:

  • The fork treats Step 6 as satisfied and lints — which defeats the gate the skill was built around ("linting only happens after the reviewer accepts, so you're not iterating on wording they haven't approved").
  • The fork blocks waiting for approval it can't obtain, and the batch stalls.

Fan-out step 7 then compounds it: "Once every fork reports a reviewer-approved diff and clean lint, do one combined Step 6 confirmation summary." A fork can never report a reviewer-approved diff, and the combined Step 6 confirmation now happens after linting, which is the reverse of the single-document flow.

Suggested fix: scope forks to Steps 2 and 5 only (resolve siblings, apply the edit), have them return the proposed diff plus any open questions, and keep Steps 3, 6, and 7 in the parent — one combined scope ask, one combined diff confirmation, then lint. That also makes fan-out step 6 ("resume the fork with SendMessage") coherent, since the fork is only ever waiting on edit-level input.

2. Concurrent forks share one working tree and can collide on the same file

.claude/skills/audit-fix/SKILL.md, fan-out steps 2–4

Every fork is deliberately put on the same branch and launched concurrently, with no isolation: "worktree" (correct — the fixes must land on one branch), but nothing prevents two forks from editing the same file. Concretely: a reviewer pastes corrections for docs/accessanalyzer/11.6/admin/foo.md and docs/accessanalyzer/11.5/admin/foo.md in one batch. Fork A resolves 11.5 as an exact-duplicate sibling and edits it; fork B has 11.5 as its primary and edits it too. Depending on interleaving, one fork's Edit either silently loses to the other or fails its exact-match search and stops to ask a question that only exists because of the race.

Worth adding a parent-side step between fan-out steps 2 and 3: run find-siblings.mjs for every source path up front, and if any two documents' resolved file sets intersect, run those documents sequentially (or merge them into one fork) rather than concurrently.

3. generate-audit-list.mjs still emits the accurate and complete columns the PR removes everywhere else

scripts/generate-audit-list.mjs:15, :506-507, :515

The PR premise is that those workbook fields are gone — template.md drops its Status section, and SKILL.md now states "There's no per-page Xchange status field or separate accurate/complete columns anymore." But the generator still writes them into every CSV:

'', // accurate
'', // complete
...
['document_title', 'version', 'live_page_url', 'source_path', 'duplicates', 'reviewer', 'audited', 'accurate', 'complete', 'notes'],

and the module docblock at line 15 still says "Review status (reviewer/audited/accurate/complete/notes) lives in the imported spreadsheet." The next npm run audit:generate produces a 10-column CSV for import into a workbook the PR says has 8. find-siblings.mjs reads columns by header.indexOf, so nothing breaks in tooling — but the import schema and the docblock are now stale against the process this PR defines. Either drop the two columns and update the docblock, or note in the PR why the CSV keeps them.

4. pingcastle: new Set(['4.0']) excludes the latest, default-version docs from the audit

scripts/generate-audit-list.mjs:61

The map's own comment scopes it to "deprecated or already-unsupported releases the Dashboard has ruled out of scope" — but PingCastle 4.0 is isLatest: true and defaultVersion: '4.0' (src/config/products.js:441-453). 4.0 has 36 doc files; 3.5 has 7. After this change, the only PingCastle pages that can ever enter the audit list are the 7 legacy 3.5 pages, and the 36 pages users actually land on by default are permanently out of scope.

Two secondary effects worth knowing about:

  • docs-audit/pingcastle/review-list.csv isn't regenerated in this PR. It happens to contain zero 4.0 rows already (presumably churn-excluded when it was last generated), so there's no artifact drift today — but the exclusion makes that permanent rather than letting the churn window expire and pick them up.
  • find-siblings.mjs enumerates siblings from products.js, not the CSV, so 4.0 files are still visible to the audit-fix skill — but since the duplicates column now comes from a 4.0-free CSV, a byte-identical 4.0 copy degrades from exact-duplicate (auto-applied) to quote-found (reviewer must approve each time). Safe, just more friction.

The Identity Manager 7.0 exclusion has none of these concerns — current is latest there, so 7.0 is genuinely a legacy version. If 4.0 is intentional, it'd help to widen the comment above the map to cover "not-yet-audited new releases" as a second reason, so the next person doesn't read it as a mistake.

Minor

  • The single-document escape hatch now reads "just work through Steps 1–7 yourself as written above", which omits Step 8 (wrap-up/commit). Harmless in practice, but "Steps 1–8" is what's meant.
  • The fan-out hard-codes subagent_type: "fork". There's no fork entry in .claude/agents/, so this depends entirely on it being available as a built-in in whatever harness a reviewer runs the skill in. Worth a fallback sentence ("if forks aren't available, work the batch sequentially") so a batch paste doesn't dead-end on a failed Agent call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant