Add doc parity to review agent

[MGibson1]

📔 Objective

Enables doc-parity plugin in review agent.

[github-actions]

Claude Configuration Validation

Reviewed the Claude material changed in this PR: the new bitwarden-doc-parity plugin (v1.0.0) and the bitwarden-code-review bump (1.13.1 → 1.14.0).

Checks run: plugin-validator agent (both plugins), skill-reviewer agent (verifying-doc-parity), reviewing-claude-config security/structure review, manual marketplace + version-consistency check, shellcheck of both hook scripts, JSON validity of hooks/evals/manifests.

Result: no critical issues. No secrets, no dangerous permissions, no malformed manifests. Structure and versioning are correct. The findings below are behavior-consistency problems — three places where the skill's text, the hook's logic, the agent's instructions, and the changelog disagree with each other.

---

Major (should fix before merge)

1. allowed-tools does not cover the skill's own workflow

plugins/bitwarden-doc-parity/skills/verifying-doc-parity/SKILL.md:7

allowed-tools: WebFetch(domain:contributing.bitwarden.com)

The workflow needs Bash(git diff:*) / Bash(git ls-files:*) (line 25), Read (Steps 2–3), Edit (line 39), and Grep/Glob for the ancestor walk. None are declared. Because the skill also sets context: fork (line 5), this line describes the forked agent's tool set. Under restrictive semantics every step except the final WebFetch is unavailable, and the failure is silent (mid-run permission denials), not a load error — which is why structural validation passes. Every other skill in this repo treats allowed-tools as the complete set it needs; the closest analogue is plugins/bitwarden-delivery-tools/skills/architecting-solutions/SKILL.md:4.

Fix: expand to the real set, e.g.

allowed-tools: Read, Edit, Glob, Grep, Bash(git diff:*), Bash(git ls-files:*), Bash(git status:*), WebFetch(domain:contributing.bitwarden.com)

or drop the field entirely. If the intent was that context: fork supplies the base toolset and this line only adds the domain-scoped WebFetch grant, state that in a comment — it contradicts the convention in the repo's other skills.

Note: step 2 of out-of-repo discovery (line 67) says "Search the contributing-docs", and eval case 10 grades exactly that search. A domain-pinned WebFetch can fetch a known URL but cannot search; WebSearch is needed for that path.

2. Skill tells the reviewer to edit files; the agent forbids it

plugins/bitwarden-doc-parity/skills/verifying-doc-parity/SKILL.md:39 vs. plugins/bitwarden-code-review/agents/bitwarden-code-reviewer/AGENT.md:87

SKILL.md:16–19 establishes two contexts (agent session, PR review), but the Step 3 Update outcome is unqualified: "fix the documentation … Edit the documentation in the same change." AGENT.md:87 now says the opposite for the review path: "Report drift as findings — do not edit files in the PR." A PR review that invokes the skill receives contradictory instructions, and the skill's text is the more specific of the two — the plausible failure is unrequested commits on a branch under review. This is fallout from commit 22aba1f, which updated the agent but not the skill.

Fix: in SKILL.md Step 3, scope Update to the agent-session context and add a review-context branch that emits a finding instead of an edit.

3. Hook and skill disagree on what makes a directory a "documented scope"

plugins/bitwarden-doc-parity/hooks/doc-parity-check.sh:113-120 vs. SKILL.md:31

	markers
SKILL.md:31	README.md, docs/, diagram sources — repo root included
documented_ancestors()	README (case-insensitive), CLAUDE.md, docs/ — repo root excluded

The hook's inline comment claims it is "aligning with SKILL.md's scope definition" — as written, it isn't. Consequences: a directory documented only by .mmd/.mermaid diagrams never arms the tripwire, and a CLAUDE.md-only scope arms the hook but is outside the rule the skill applies. The evals already grade the hook's version, not the skill's: case 2 (behavior-eval.json:26) and case 7, named instruction-files-count-as-documentation (behavior-eval.json:89), both require treating util/Seeder/CLAUDE.md as documentation — behavior the skill never states.

Fix: add instruction files to SKILL.md:31 (e.g. "contains a README.md, a CLAUDE.md or equivalent instruction file, a docs/ directory, or diagram sources"), then correct the stale comments in doc-parity-check.sh:7-9 and :113-114. The root-scope exclusion is deliberate and documented at doc-parity-check.sh:8-10 — worth stating in SKILL.md too, so the two layers read as one rule.

---

Minor (should fix)

4. bitwarden-code-review changelog describes behavior the PR removed

plugins/bitwarden-code-review/CHANGELOG.md:12 — the 1.14.0 entry still reads "verify or update in-repo docs at every documented ancestor scope", the exact wording commit 22aba1f replaced in AGENT.md with report-only. Since 1.14.0 is introduced by this PR, amend the entry in place rather than adding a new one. This is the same code-vs-docs drift class the plugin exists to catch.

5. Any .md touch grants coverage to an entire subtree

plugins/bitwarden-doc-parity/hooks/doc-parity-check.sh:65-71 (is_doc_file), :75-82 (doc_scope)

is_doc_file matches *.md unconditionally, so editing a CHANGELOG.md, a scratch note, or a markdown test fixture under src/ yields scope src and satisfies coverage for every changed file below it. That is precisely the "token documentation edit" the hook's own block message forbids (line 149), reachable without intent.

Fix: restrict the coverage-granting set to README.md, CLAUDE.md, docs/**, and diagram sources.

6. jq --rawfile breaks the SessionStart hook's fail-open contract on jq < 1.6

plugins/bitwarden-doc-parity/hooks/doc-parity-context.sh:19 (contract stated at lines 8-9)

The script guards on command -v jq but not on version; --rawfile landed in jq 1.6. On jq 1.5 the hook exits non-zero and surfaces an error instead of proceeding silently.

Fix: jq -n --arg ctx "$(cat "$FRAGMENT")" ... (works on 1.5+), or append || exit 0.

7. Marker directory in shared TMPDIR is not ownership-checked

plugins/bitwarden-doc-parity/hooks/doc-parity-check.sh:37-40, :146

The -O guard at line 40 protects the marker file, but MARKER_DIR="${TMPDIR:-/tmp}/doc-parity" is created with default perms and never ownership-verified. On a multi-user host with TMPDIR unset, a pre-existing /tmp/doc-parity owned by another user (or symlinked elsewhere) redirects the touch at line 146 and exposes session IDs.

Fix: mkdir -m 700, then [[ -d "$MARKER_DIR" && -O "$MARKER_DIR" ]] || MARKER="" before use.

8. Once-per-session guarantee degrades silently when session_id is absent

plugins/bitwarden-doc-parity/hooks/doc-parity-check.sh:34-43

If .session_id is missing from the payload, MARKER stays empty, nothing is recorded, and only the stop_hook_active guard prevents an immediate re-block — so the hook can block again later in the same session, contrary to the header comment at lines 12-13. Fall back to a PPID- or cwd-derived marker, or document the degradation.

9. Per-file process spawning can exceed the 15 s Stop-hook timeout

plugins/bitwarden-doc-parity/hooks/doc-parity-check.sh:115-125, timeout at hooks/hooks.json:20

documented_ancestors forks dirname twice per path component and runs compgen -G plus two filesystem tests per ancestor, for every changed non-doc file. A few thousand changed files in a deep tree is tens of thousands of forks; on timeout the hook is killed and the check silently does nothing. Memoize the per-directory answer (bash 4 assoc array, or a sorted-string cache to stay bash 3.2-compatible for macOS), and/or cap files inspected.

10. evals/README.md claims a case that does not exist

plugins/bitwarden-doc-parity/skills/verifying-doc-parity/evals/README.md:5 claims coverage of "root-level placement for a repo-wide capability." The ten cases are: enumerate-ancestors, attestation-happy-path, refuses-checkbox-theater, fixes-contradicting-docs, deletion-is-doc-maintenance, drift-two-scopes-up, instruction-files, dismisses-tripwire, placement-follows-doc, review-context. Case 9 (behavior-eval.json:115) tests the opposite — that a doc should not be hoisted to the repo root. The corresponding SKILL.md:39 rule ("A scope's documentation describes what is present at that scope and below…") is therefore ungraded.

Fix: add the case and regenerate the baseline, or drop the claim.

11. Smaller items

• Second-person drift — SKILL.md:39: "isn't obvious from what you already have" is the file's only second-person usage. Rewrite as "…from the material already gathered."
• Description length — SKILL.md:3: 560 chars, above the ~500-char guidance (well under the 1024 limit). Trim "even if the request does not name a skill or documentation explicitly" rather than any trigger phrase.
• Unverified frontmatter fields — SKILL.md:4-6: agent: general-purpose, context: fork, background: false appear in no other SKILL.md in this repo. Legitimate forked-skill fields, but confirm CI's validate-plugin-structure.sh accepts them, and note in the README's Layer 3 section why this skill forks when others don't.
• Zeroed baseline metrics — behavior-baseline.json: run_summary.time_seconds and all tokens are 0. The regression projection at evals/README.md:40 correctly excludes them, but a future reader may read zeros as measurements — populate or annotate.
• Time-sensitive prose — evals/README.md:5: "each recently-added instruction" won't age; name the rules instead.
• Eval case 8 rationale — behavior-eval.json:98 says the file is code "because it isn't a .md/.mdx/.mmd file"; is_doc_file() also matches *.mermaid. Cosmetic, but the prose is grading context.

12. Pre-existing, out of scope for this PR

• plugins/bitwarden-code-review/commands/code-review/README.md and commands/code-review-local/README.md sit inside commands/ and are auto-discovered as slash commands (no frontmatter → description from first line). Consider relocating.
• plugins/bitwarden-code-review/.claude-plugin/plugin.json:22-25 lists command paths that commands/ auto-discovery already picks up. The agents entry on line 21 is legitimate (agents/<dir>/AGENT.md is not an auto-discovery shape).
• No AGENT.md in this repo uses <example> blocks or color; noting for completeness only, since it's a repo-wide convention rather than a defect in this plugin.

---

Verified clean

Security

• No secrets, API keys, tokens, or hardcoded credentials in any changed file.
• No settings.local.json tracked in git.
• plugins/bitwarden-code-review/.claude/settings.json is deny-only, blocking the mutating gh surface (merge/close/edit/release/secret/workflow, all non-GET gh api methods). No dangerous auto-approvals, no broad file-access grants.
• Hook scripts: no eval, no unquoted expansion of payload data; session_id scrubbed with tr -cd 'a-zA-Z0-9_-' before use in a path; shellcheck -S style clean on both.

Structure and manifests

• Both plugin.json files: valid JSON, kebab-case names, valid semver, required fields present.
• Marketplace consistency: .claude-plugin/marketplace.json matches both manifests exactly on name, version, description, and source path.
• Version consistency across all four required locations — marketplace.json, plugin.json, README.md catalog rows (lines 13, 18), and AGENT.md frontmatter (version: 1.14.0).
• CHANGELOG.md present for both, entries matching the shipped versions (see finding 4 for the entry's wording).
• Auto-discovery layout correct: hooks/hooks.json, skills/*/SKILL.md, agents/, commands/.

Hooks

• hooks.json uses the {"hooks": {...}} plugin wrapper; SessionStart and Stop are valid events; omitting matcher is correct for both; timeouts set (5 s / 15 s).
• Both commands use ${CLAUDE_PLUGIN_ROOT} with quoted paths, invoked via bash; the executable bit is committed anyway (100755).
• Stop hook uses the correct {"decision":"block","reason":...} + exit 0 contract; SessionStart uses the correct hookSpecificOutput.hookEventName / additionalContext shape.
• Fail-open paths verified for missing jq, non-repo cwd, and empty diff; the pipe-to-while subshell bug is avoided via here-strings; core.quotePath=false handles non-ASCII paths.

Skill and evals

• name matches directory; description is third-person with concrete trigger phrases; body is 792 words — lean enough that no references/ or examples/ split is warranted.
• Only internal reference is the anchor #out-of-repo-discovery-review-context-only (SKILL.md:19), which resolves to the heading at SKILL.md:62. All README relative links resolve.
• Both eval JSON files parse. Baseline is internally consistent: 100 runs = 10 cases × 5 runs × 2 configurations; every expectation text matches behavior-eval.json; passed + failed == total and pass_rate recompute for all 100 runs; summary means/stddev reproduce (with_skill 0.987 / 0.0523, without_skill 0.808 / 0.2594, delta +0.18).
• corpus_ref d6c84a7562cc6b464de910dbf829690885500137 is identical across behavior-eval.json, behavior-baseline.json, and evals/README.md:19.
• All three of the skill's outcomes (Update / Attest / Dismiss) have dedicated cases. The deliberate omission of trigger evals is documented with a reason (evals/README.md:9-13) rather than left as a silent gap.

---

Checks that could not run in this environment

• pnpm run lint (prettier + cspell) — pnpm is not installed and npx is blocked by the repo's devEngines.packageManager pin. Formatting and spelling were not verified; CI's lint.yml will cover them.
• validate-plugin-structure.sh / validate-marketplace.sh — these live in bitwarden/gh-actions and no checkout was available. Their checks (structure, marketplace consistency, version bump across all four files) were performed manually instead and pass.

Note that plugin-dev's bundled validate-hook-schema.sh reports "Unknown event type: hooks" and then crashes on this hooks.json. That script does not understand the plugin-level {"hooks": {...}} wrapper — the config is correct and matches bitwarden-ai-telemetry; the validator script is stale.

---

Recommendation

Request changes on findings 1–3 (tool declaration, edit-vs-report conflict, scope-definition mismatch) and finding 4 (changelog wording). Findings 5–11 are quality items that can follow.

Per .claude/CLAUDE.md, fixes to findings 1, 2, 3, and 5–9 are substantive changes to bitwarden-doc-parity and need a version bump to 1.0.1 plus a CHANGELOG.md entry, mirrored in .claude-plugin/marketplace.json, plugins/bitwarden-doc-parity/.claude-plugin/plugin.json, and the README.md catalog row. Finding 4 is an in-place amendment to the unreleased 1.14.0 entry and needs no further bump.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR wires the optional bitwarden-doc-parity plugin into the bitwarden-code-reviewer agent as a Cross-Plugin Enrichment step, and bumps the plugin to 1.14.0. The version bump is correctly applied across all four required files (marketplace.json, plugin.json, root README.md, AGENT.md) with a matching changelog entry, and the new entry follows the structure of the existing enrichment bullets including the optional-dependency fallback. One wording concern was raised about the review-context instruction.

Code Review Details

• ⚠️ : Enrichment bullet instructs the read-only review agent to "update in-repo docs", which the agent cannot do in the CI review workflow and which contradicts the doc-parity plugin's documented review-face behavior
  • plugins/bitwarden-code-review/agents/bitwarden-code-reviewer/AGENT.md:87

[claude]

⚠️ IMPORTANT: "update in-repo docs" tells a read-only review pass to edit the repo

Details and fix

The reviewer agent is a read-only reviewer — its tools: list has no Edit, and in the CI review workflow any file it did write would be silently discarded (never committed or pushed). Instructing it to "verify or update in-repo docs" imports the session-context behavior of verifying-doc-parity Step 3 ("fix the documentation… Edit the documentation in the same change"), which has no review-context carve-out. The likely outcome is a summary that attests updated: … for scopes where nothing was actually changed on the PR branch — or, if the forked general-purpose skill agent does have write access, unexpected mutations to the author's checkout during a review.

This also contradicts the doc-parity plugin's own description of its review face: "Changes that invalidate out-of-repo docs get called out at review."

Suggested wording:

- invoke `Skill(verifying-doc-parity)` to run the documentation pass in review context: identify drifted in-repo docs at every documented ancestor scope of the change, and discover out-of-repo pages the change invalidates. Report drift as findings — do not edit files in the PR — and fold them into your classification and validation in Steps 3–4, applying the documentation standard's external-docs flow (work item before merge, stale marker on the page) when applicable.

Note the suggested text also routes findings through Steps 3–4 (confidence scoring and false-positive validation), matching the reviewing-claude-config entry directly above; as written, "fold findings into your report" bypasses those gates.

[MGibson1]

982587f