Add bitwarden-test-toolkit plugin with assessing-test-coverage skill#169
Add bitwarden-test-toolkit plugin with assessing-test-coverage skill#169nthompson-bitwarden wants to merge 9 commits into
Conversation
Plugin Validation Summary —
|
| Check | Result |
|---|---|
plugin.json manifest (name, version, required fields, semver) |
✅ Valid JSON; kebab-case name; 1.0.0 valid semver; well-formed author/homepage/repository/keywords |
| Version consistency (plugin.json ↔ marketplace.json ↔ CHANGELOG.md) | ✅ 1.0.0 agrees across all three |
| name/description consistency (plugin.json ↔ marketplace.json) | ✅ Identical |
marketplace.json source path resolves |
✅ ./plugins/bitwarden-test-toolkit |
| Directory structure / auto-discovery | ✅ Standard layout; skill under skills/<name>/SKILL.md |
| Skill frontmatter (name, description, argument-hint, allowed-tools) | ✅ Valid; name matches directory |
| File organization (README present, no stray files) | ✅ Comprehensive README; eval assets cleanly namespaced under evals/ |
| CHANGELOG format (Keep a Changelog) | ✅ Single Added section for initial release |
Cross-plugin dependency (bitwarden-atlassian-tools) |
✅ Exists in marketplace |
| Hardcoded credentials | ✅ None |
No commands, agents, hooks, or MCP servers are present (none required for this plugin).
2. Skill Review (skill-reviewer) — ✅ PASS
Skill: skills/assessing-test-coverage/SKILL.md
| Check | Result |
|---|---|
| YAML frontmatter (required name, description) | ✅ Valid; delimited correctly |
| Description quality | ✅ Excellent — third-person "Use when…", concrete quoted trigger phrases, explicit negative-scope clause ("Do NOT use it to…") disambiguating from sibling test skills |
| Writing style | ✅ Consistently imperative/infinitive |
| Word count | |
| Progressive disclosure | ✅ Lean SKILL.md with short inline output template; no references//examples//scripts/ needed at this size |
| Referenced files exist | ✅ All eval files referenced in evals/README.md exist; ${CLAUDE_PLUGIN_DATA} is an established repo convention resolved at runtime |
| Anti-patterns | ✅ None (no vague triggers, no bloat) |
| Prompt-injection guardrail | ✅ Treats Jira/Confluence/PR/CSV content as untrusted (CWE-1427) |
Eval harness (evals/) |
✅ Positive — documented custom trigger-rate runner with 10/10 should-trigger + 10/10 should-not-trigger baseline; validates the description's negative scope |
3. Security Validation — ✅ PASS
| Check | Result |
|---|---|
| Committed secrets (API keys, tokens, passwords) | ✅ None — only match is the benign constant TARGET_SKILL_TOKEN = "assessing-test-coverage" in run_real_eval.py:22 |
| Hardcoded credentials in code | ✅ None |
allowed-tools scoping |
✅ Tightly scoped least-privilege globs (e.g. Bash(gh pr view:*), Bash(git -C * rev-parse:*)) rather than blanket Bash |
run_real_eval.py subprocess safety |
✅ Uses subprocess.Popen with an argument list (no shell=True), strips CLAUDECODE from env, and early-bails on real-work tools to prevent resource exhaustion — no injection or unsafe shell usage |
| Untrusted-input handling | ✅ SKILL.md explicitly instructs treating external content as data, not instructions |
Warnings (should fix — non-blocking)
- [minor / warning]
plugins/bitwarden-test-toolkit/README.md:18(also:87,:96) — README lists the report sections as "Overview, Summary, Evidence, Coverage, and Gaps", but the authoritative output template inSKILL.md:37-65defines only## Overview,## Evidence & sources,## Coverage, and## Gaps(noSummary). Fix: drop "Summary" from the README, or add a## Summaryblock to the SKILL.md template so the two agree. - [minor / warning]
plugins/bitwarden-test-toolkit/skills/assessing-test-coverage/evals/README.md:20-27— the documented run command uses relative paths (--eval-set trigger-eval.json) that only work from withinevals/. Fix (optional): note the required working directory, or use path-agnostic invocation. Doc-only; the script is a helper, not a plugin component.
Optional suggestion
SKILL.md:27— consider a one-line gloss for${CLAUDE_PLUGIN_DATA}(e.g. "the plugin's per-user data directory") for standalone readability. Not required; it is an established repo convention.
Overall Assessment
Ready to merge. Manifest correctness, cross-file version consistency (1.0.0), skill frontmatter/quality, progressive disclosure, and security all pass with zero errors. The two findings are minor documentation inconsistencies that should be tidied but do not block acceptance.
Note: the repo validation scripts (
validate-plugin-structure.sh,validate-marketplace.sh) could not be executed in this environment (command approval unavailable); their checks were performed manually and via the plugin-validator agent, all passing.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Inline the previous references/ content into a lean, self-contained SKILL.md step list and delete the reference files. Tighten the skill description with explicit scope exclusions to sharpen triggering, add a prompt-injection guard for untrusted input (CWE-1427), and allow git clone so uncloned repos can be inspected before being marked unverified. Rework the trigger eval with a portable runner that watches for the real installed skill, refresh the README, and re-record the baseline. Latest run: 10/10 should-trigger, 10/10 should-not-trigger.
| - **Evidence-grounded inventory**: Reports only coverage it can observe and cite — a behavior with no observed test is recorded as a gap, never assumed covered. | ||
| - **PRs-first discovery**: Takes tests in linked/merged PR diffs as the primary, permalink-ready evidence, then a targeted lookup scoped to the change surface — never a repo-wide sweep. | ||
| - **Layer bucketing**: Sorts each observed test into unit / integration / E2E per repo, using each repo's own conventions. | ||
| - **Stable permalink citations**: Cites 1–3 representative tests per behavior as commit-SHA GitHub permalinks (not branch links), plus an approximate count. |
There was a problem hiding this comment.
♻️ DEBT: Permalink guidance here contradicts the operative SKILL.md.
Details
This README states citations should be "commit-SHA GitHub permalinks (not branch links)" (also line 36, and the workflow diagram at line 65 says "permalink-ready via head SHA"). But SKILL.md:32 (the file that actually drives behavior) says the opposite:
Cite tests on the repo's current default branch, not at a PR-head SHA — merged code may have been reverted; a PR-head permalink still resolves but can point at tests no longer on the branch.
So the README tells a reader to prefer commit-SHA / head-SHA links, while the skill instruction warns against exactly that (PR-head SHA) and prefers default-branch links. A maintainer reading the README will document/expect the wrong behavior.
Suggest aligning the README wording (and the diagram's "permalink-ready via head SHA" node) with the SKILL.md rule: cite on the current default branch, not a PR-head SHA.
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/QA-1983
📔 Objective
Add the bitwarden-test-toolkit plugin to the marketplace, providing an assessing-test-coverage skill that analyzes a repository's existing test stack and reports coverage gaps — with supporting references and evals, plus the required marketplace registration and dictionary/README updates.