Repository navigation
[QA-2464] feat(bitwarden-testing-tools): review existing Testmo cases in assessing-test-coverage - #307
[QA-2464] feat(bitwarden-testing-tools): review existing Testmo cases in assessing-test-coverage#307awiester-bw wants to merge 3 commits into
Conversation
…ing-test-coverage
Claude Code validationResult: Pass Re-review after the author's fix commits ( Not covered: Both Findings
Findings 1, 2, 7, 8, 9, and 10 are new this round and are posted as inline comments with reasoning and a fix. The nine marked "already raised" keep their existing unresolved threads and were not re-posted. Dropped after documentation check: three claims from the component checkers, each contradicted by the official documentation.
Two further layout nits were dropped as not worth a reviewer's time: a missing Resolved since the last reviewAll six were fixed in
A seventh, unprompted fix corrected a stale "Skill C" reference to "This skill" at What passed
|
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
There was a problem hiding this comment.
15 Open source vulnerabilities detected - high severity
Aikido detected 15 vulnerabilities across 5 packages, it includes 5 high, 6 medium and 4 low vulnerabilities.
Details
Remediation:
@modelcontextprotocol/sdk— 7 CVEs (high) — fixed in 1.32.0body-parser— 1 CVE (high) — fixed in 2.4.0zod— 2 CVEs (high) — fixed in 4.6.6fast-uri— 4 CVEs (medium) — fixed in 3.1.9raw-body— 1 CVE (low) — fixed in 4.0.0
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@modelcontextprotocol/sdk 1.31.0 and zod 4.5.4. Those are the vulnerable versions listed above, and the sibling bitwarden-atlassian-mcp-server in this repo already uses the fixed versions.
Details and fix
plugins/bitwarden-atlassian-tools/mcp/bitwarden-atlassian-mcp-server/package.json already pins @modelcontextprotocol/sdk 1.32.0 and zod 4.6.6. Those are the fix versions Aikido reports, and that server also clears the 7-day minimumReleaseAge gate. This package's package.json (lines 25 and 28) copies the older pins:
"@modelcontextprotocol/sdk": "1.32.0",
"zod": "4.6.6"After that, run pnpm install again to regenerate pnpm-lock.yaml. The sibling's pnpm-workspace.yaml also has overrides for transitive packages pulled in by the SDK. Check whether body-parser, fast-uri, and raw-body need the same treatment here.
This server only uses stdio, so most of the SDK's HTTP-transport CVEs probably can't be reached in practice. Still, the bundled server ships to every plugin user, and the fix is the same version bump the repo has already made elsewhere.
code-review
There was a problem hiding this comment.
Resolved at the current head. package.json now pins @modelcontextprotocol/sdk 1.32.0 and zod 4.6.6, and pnpm-lock.yaml resolves body-parser 2.4.0 and fast-uri 3.1.9. Those match the fix versions listed above and the sibling bitwarden-atlassian-mcp-server. raw-body resolves to 3.0.2, the same version the sibling server ships.
code-review
| const platform = detectPlatform(path); | ||
| const hits: CodeHit[] = []; | ||
|
|
||
| lines.forEach((line, index) => { |
There was a problem hiding this comment.
The forEach callback combines match collection, attribute analysis, and hit construction, making the scan flow harder to follow as an unnamed function.
Details
✨ AI Reasoning
The callback iterates over each source line, gathers matches from three patterns, determines attribute state, and creates a hit for each ID. These are substantial pieces of scanning logic inside an unnamed callback, so a clear name or explanatory comment would improve readability.
🔧 How do I fix it?
Extract complex anonymous functions into named functions with descriptive names, or add explanatory comments for their purpose.
Reply on this comment to give feedback or ignore the issue.
More info
| const magic = field(header, 257, 6); | ||
| const prefix = magic.startsWith("ustar") ? field(header, 345, 155) : ""; | ||
| const name = field(header, 0, 100); | ||
| const path = pendingPath ?? (prefix === "" ? name : `${prefix}/${name}`); |
There was a problem hiding this comment.
The pendingPath ?? expression contains a ternary fallback, requiring readers to evaluate two conditional selections in one expression.
| const path = pendingPath ?? (prefix === "" ? name : `${prefix}/${name}`); | |
| const archivePath = prefix === "" ? name : `${prefix}/${name}`; | |
| const path = pendingPath ?? archivePath; |
Details
✨ AI Reasoning
The expression first selects whether to use pendingPath, then conditionally chooses between name and the prefixed path. Evaluating both selection steps on one line adds avoidable cognitive complexity.
Reply on this comment to give feedback or ignore the issue.
More info
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed the current head, including the latest commits that bump Not covered: Skill review did not run. This review path cannot launch Code Review DetailsNo new findings. Dependency Changes
All six are new to this package only. The same versions already ship in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ## Steps | ||
|
|
||
| Before the first read from Jira or Testmo, load the tools by exact name with ToolSearch: `select:mcp__plugin_bitwarden-atlassian-tools_bitwarden-atlassian__get_issue` for Jira, and `select:mcp__testmo__get_states,mcp__testmo__get_statuses,mcp__testmo__get_repository_folders,mcp__testmo__get_repository_folder,mcp__testmo__get_repository_cases,mcp__testmo__get_repository_case,mcp__testmo__get_case_result_history` for Testmo. Do not use keyword searches for these tools, since they can match unrelated tools. A keyword search that finds nothing does not mean the tools are missing. If an exact-name load finds nothing, try the same load once more before doing anything else, because the plugin's servers can take a moment to start. Report a source as unavailable only when that second load or a call to the tool itself fails, and quote the error. |
There was a problem hiding this comment.
find_automated_cases, the one tool step G calls.
Details and fix
This line establishes the rule that MCP tools must be loaded by exact name before use, and lists select: entries for the Atlassian tool and all seven mcp__testmo__* tools. It does not list mcp__plugin_bitwarden-testing-tools_bitwarden-test-automation__find_automated_cases, which step G (line 133) then calls.
That tool is the one most likely to be missing at first call. Its server launches via pnpm install --frozen-lockfile --prod and a tsc build (plugins/bitwarden-testing-tools/.mcp.json:8), which is exactly the "the plugin's servers can take a moment to start" case this line warns about. And unlike step A, which has an explicit fallback for Testmo being unreachable, step G only handles the tool returning Code not checked — there is no instruction for the tool not being registered at all. A cold-start miss therefore degrades into a silent skip of the Automation Type check rather than a NOT CHECKED line.
Append the tool to the exact-name select: list:
select:mcp__testmo__get_states,mcp__testmo__get_statuses,mcp__testmo__get_repository_folders,mcp__testmo__get_repository_folder,mcp__testmo__get_repository_cases,mcp__testmo__get_repository_case,mcp__testmo__get_case_result_history,mcp__plugin_bitwarden-testing-tools_bitwarden-test-automation__find_automated_cases
validate-ai
| 5. Run the Testmo procedure below for every story. If Testmo cannot be read, record why in the Testmo section and continue with the code side. | ||
| 6. Write the report to `${CLAUDE_PLUGIN_DATA}/coverage-reports/<slug>-<timestamp>-coverage.md` (`<slug>` from the ticket/PR/feature; `<timestamp>` from `date +%Y-%m-%d-%H%M%S`) using the template below. | ||
|
|
||
| ## Testmo procedure |
There was a problem hiding this comment.
references/ directory.
Details and fix
Measured on the merge base and head:
git show 8aa784a:…/assessing-test-coverage/SKILL.md | wc -w # 635
wc -w …/assessing-test-coverage/SKILL.md # 4398
The "Testmo procedure" (steps A–J, lines 43–184) and the "Testmo section layout" (lines 223–287) are roughly 3,700 new words added inline. All of it loads into context on every invocation of the skill, including the parts that only matter once cases have already been found — the entry layouts, the confidence checklist, the per-section ordering rules.
The growth is entirely introduced by this changeset, so this is not a pre-existing length problem.
Move "Testmo procedure" and "Testmo section layout" into references/testmo-case-review.md and leave a one-line pointer at step 5. The plugin already has this pattern in-tree at plugins/bitwarden-testing-tools/skills/setting-up-regression-runs/references/, so the layout is established.
Reference: https://code.claude.com/docs/en/skills
validate-ai
| description: Use when determining what test coverage ALREADY exists for a specific change (a PR, Jira key, Tech Breakdown doc, Testmo CSV, changed paths, or named component). Triggers on "what's already tested", "does this PR have tests", "what coverage exists for", "is this component covered", or "which behaviors have no test today". This is a backward-looking inventory of existing coverage for a concrete change. Do NOT use it to recommend or decide which new tests to add ("should I add integration tests here", "are unit tests enough"), to design a test strategy or plan, to run or fix existing tests, or to explain testing concepts like the test pyramid or which layers a repo uses — those are all out of scope. | ||
| argument-hint: "[PR URL | Jira key | Tech Breakdown doc | Testmo CSV]" | ||
| allowed-tools: "Read, Write, Grep, Glob, Bash(date:*), Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh api repos/bitwarden/*), Bash(gh search code:*), Bash(git rev-parse:*), Bash(git remote get-url:*), Bash(git -C * rev-parse:*), Bash(git -C * remote get-url:*), Bash(git clone:*), Skill(bitwarden-atlassian-tools:researching-jira-issues)" | ||
| description: Use when determining what test coverage ALREADY exists for a specific change (a PR, Jira key, Tech Breakdown doc, Testmo CSV, changed paths, or named component), or which existing Testmo cases and tests it makes obsolete, wrong, or duplicated, including other teams' cases. Use it before writing new tests or Testmo cases. Triggers on "what's already tested", "does this PR have tests", "what coverage exists for", "is this component covered", "which behaviors have no test today", "which Testmo cases does this story make obsolete", or "which tests should be retired or updated for PM-123". A backward-looking review: it recommends update or retire, lists possible duplicates, notes other teams' affected cases, and never writes to Testmo or code. Do NOT use it to decide which new tests to add ("should I add integration tests here", "are unit tests enough"), to design a test strategy or plan, to run or fix existing tests, or to explain testing concepts like the test pyramid or which layers a repo uses. |
There was a problem hiding this comment.
Details and fix
The rewritten description claims the capability — "it recommends update or retire, lists possible duplicates, notes other teams' affected cases" — but every phrase in the Triggers on list is about coverage, obsolescence, or retirement. None is duplicate-shaped.
The PR adds an eval query for exactly this capability and records it failing:
"query": "PM-41861 and PM-42581 have the same title -- did we end up with two sets of Testmo cases for the same thing?",
"trigger_rate": 0.0plugins/bitwarden-testing-tools/evals/README.md:104 confirms it is also 0/7 on claude-opus-5-5, so it is not a single-model artifact. Unlike the RestoreAsync, passkey, and cipher-key-rotation queries, this one has no "before this change" measurement showing it was already failing — it is new, for a capability this PR is adding.
The plugin's own policy at evals/README.md:92 says to "fix the skill description rather than the eval set" when a query drops below threshold. Here the failure was documented instead of fixed, so a headline capability of the PR will not route.
Add a duplicate trigger phrase to the Triggers on list, for example "do we have two Testmo cases for the same thing", then re-record evals/baseline.json.
validate-ai
| - No em dashes anywhere in the output. Use a colon, a comma, or a new sentence instead. | ||
| - The Evidence & sources row lists the folders searched and says "searched by case name only." | ||
| - Readers may not have a Testmo seat, so each entry stands on its own: link, folder, why, and the note to paste or the proposed text. | ||
| - HEADS-UP FOR OTHER TEAMS lists only heads-up cases that need an update or retire under step E. A heads-up case that needs a change is listed there, not under SUGGESTED CHANGES. |
There was a problem hiding this comment.
Details and fix
This line says HEADS-UP FOR OTHER TEAMS "lists only heads-up cases that need an update or retire under step E."
"Heads-up case" is defined at line 87 as a case whose Team field names other teams but not the story's team. Line 86 puts a shared case — Team names the story's team and others — in full-review scope instead, so a shared case is by definition not a heads-up case.
But line 123 requires exactly that:
When a full-review case is shared … and gets a retire or update, each other team on the case also gets a heads-up line in the HEADS-UP FOR OTHER TEAMS section.
So line 123 mandates entries that line 235's "only" excludes. A model weighting line 235 produces no cross-team notification for shared cases, which is the single case where another team most needs one. Both lines are new in this PR.
Change line 235 to admit both sources:
- HEADS-UP FOR OTHER TEAMS lists heads-up cases that need an update or retire under step E, plus one line per other team on a shared full-review case that needs a change. A heads-up case that needs a change is listed there, not under SUGGESTED CHANGES.
validate-ai
|
|
||
| F. **Read the history of each retire or update candidate.** Call `get_case_result_history` with project ID 1 and the case ID for the last run date and result. Results come back as status IDs. Call `get_statuses` with project ID 1 once per run to turn them into names, such as Passed or Failed. Keep the number of candidates small. Each call is one request. | ||
|
|
||
| G. **Check Automation Type against code.** Run this for every full-review case, including ones the story does not change, because a mismatch is a finding either way. Call `find_automated_cases` with the case IDs, up to 50 per call. It reads `bitwarden/test` on its main branch through GitHub, never a local clone. For each ID it returns found or not found, and for each match the file, line, attribute, platform, and whether the test is ignored or has a KnownBug. |
There was a problem hiding this comment.
Details and fix
This line opens with:
Run this for every full-review case, including ones the story does not change, because a mismatch is a finding either way.
Line 149 closes the same step with:
Check only the IDs you fetched in full-review scope.
Those are different sets, because step C at line 82 explicitly says Do not fetch every case in the results. Only a subset is ever fetched, so "every full-review case" and "the IDs you fetched" can never coincide, and which one a run follows decides how much of the Automation Type check actually happens.
Line 133's version is also the cheaper one to honour: find_automated_cases takes case IDs only and accepts up to 50 per call, so it needs nothing from get_repository_case. Fetching is irrelevant to it.
Settle it on line 149 in favour of the wider set:
Report mismatches only, for the team to decide. Do not call them bugs. Check every full-review case ID from step C's merged results, fetched or not. If the tool returns "Code not checked", list the cases as not checked for code, with the reason the tool gave. Never state that a case has no test in code unless the tool returned that result.
validate-ai
|
|
||
| The "Coverage now" part only tells the team where the behavior is tested. It is not the reason for the retire, so it may name newer cases. The "why" part may not. | ||
|
|
||
| F. **Read the history of each retire or update candidate.** Call `get_case_result_history` with project ID 1 and the case ID for the last run date and result. Results come back as status IDs. Call `get_statuses` with project ID 1 once per run to turn them into names, such as Passed or Failed. Keep the number of candidates small. Each call is one request. |
There was a problem hiding this comment.
Details and fix
This step requires get_case_result_history plus get_statuses for "each retire or update candidate".
The Retire layout has somewhere to put that:
Last run: <date and result, or "no results recorded"> # line 248
The Update layout (lines 252–260) has no such field, and line 231 closes off improvising one:
An entry holds only the fields in its layout. No parenthetical remarks, and no comments after an entry.
So every update candidate costs a Testmo request whose result must then be discarded — in a procedure that warns about rate limits at line 183 and tells the model at line 131 to "Keep the number of candidates small. Each call is one request."
Add the field to the Update layout at line 258, matching the Retire layout:
Automation Type: <type, only when relevant>
Last run: <date and result, or "no results recorded">
Now: <current step or expected result>
Proposed: <proposed step or expected result>
validate-ai
|
|
||
| 10. Before writing the plan, check that every behavior AC (scope ACs excluded) appears in at least one test row or in the Unspecified table. Add any missing AC to the Unspecified table with "no test derived" in the Source column. Write the plan using the template below to `${CLAUDE_PLUGIN_DATA}/designing-test-plans/<key>-<timestamp>-test-plan.md`. Get `<timestamp>` by running `date +%Y-%m-%d-%H%M%S` as a single command with nothing else in it (no echo, pipes, or command substitution), then use its output. State the full path when done. If the write fails, say so and return the full plan inline instead of a path. | ||
|
|
||
| 11. Hand the saved plan to coverage. Only when the user's request also asks about existing tests or Testmo cases, and only after the plan is saved, invoke `Skill(bitwarden-testing-tools:assessing-test-coverage)` with the key and the saved plan's full path. Pass only those two, never the plan's contents. If the plan write failed, do not invoke it, and tell the user the coverage review needs a saved plan. If the call fails, report it and keep the plan path in your answer. Report the coverage result's report path along with the plan path. |
There was a problem hiding this comment.
Details and fix
Step 11 activates only when the request "also asks about existing tests or Testmo cases". But the description at line 3 — unchanged by this PR — tells the router:
Do NOT use it … to inventory existing tests (use assessing-test-coverage)
So "design the tests for PM-123 and tell me which cases we already have" is steered to assessing-test-coverage, which designs no plan. The user lands in the coverage skill with no plan to overlap against, and the handoff this PR adds never runs.
This is a reachability problem the PR introduces: before step 11 existed, the line 3 exclusion was simply correct. Now it suppresses the new feature's only trigger condition.
Amend the exclusion on line 3 so it carves out the combined request:
to inventory existing tests on their own (use assessing-test-coverage; this skill hands the saved plan to it when the same request also asks what already exists)
Note this touches the description, so per plugins/bitwarden-testing-tools/evals/README.md:70-77 the eval needs re-running for both designing-test-plans and assessing-requirement-readiness, with the refreshed baseline.json committed.
validate-ai
|
|
||
| ```bash | ||
| read -s TESTMO_TOKEN # paste your key, then press Enter | ||
| claude mcp add testmo "https://testmo.sembi.com/mcp?mode=read-only&INSTANCE-URL=bitwarden.testmo.net" --transport http --scope user --header "Authorization: Bearer $TESTMO_TOKEN" |
There was a problem hiding this comment.
🎨 SUGGESTED: The documented setup expands the Testmo API key into process argv, where any local process can read it.
Details and fix
$TESTMO_TOKEN is in double quotes, so the shell expands it before claude is executed. The plaintext key becomes an element of the command's argument list, readable by any local process through ps or /proc/<pid>/cmdline for the duration of the call.
The read -s on line 55 is good — it keeps the key out of shell history and off the screen — but it does not cover argv. The README notes the key is saved to ~/.claude.json, which is the at-rest exposure, but not this one.
The repo's own bundled-server pattern avoids it: plugins/bitwarden-atlassian-tools/.mcp.json:11-15 stores literal ${VAR} strings and lets Claude Code expand them at server launch.
Single-quote the header so the literal placeholder is stored and expanded at launch instead:
export TESTMO_TOKEN=... # from your shell profile, not pasted inline
claude mcp add testmo "https://testmo.sembi.com/mcp?mode=read-only&INSTANCE-URL=bitwarden.testmo.net" --transport http --scope user --header 'Authorization: Bearer ${TESTMO_TOKEN}'validate-ai
|
|
||
| Never search or recommend cases in **Retired Test Cases** or **Need to be deleted**. Skip any result whose folder path starts with either name. Cases in those two folders are not reviewed, even when they are Active. | ||
|
|
||
| C. **Find related cases.** Call `get_repository_cases` once per client folder and once for New Release, with `folder_id` set to that folder, `recursive` true, `state_id` set to every state except Retired, `sort` set to `repository_cases:name`, `order` set to `asc`, and `per_page` 25. Make one call per distinctive term from the acceptance criteria, using `name`. Testmo matches the case name only, partially and without regard to case, and does not search case bodies. A case whose name shares no term with the ACs will not show up. Each result includes the case's ID, name, folder ID, state, Team, Automation Type, Needs Review, Remove After Release, and description. Merge the results by case ID. If a search returns more than one page, narrow it with a more specific name before reading more pages. For the folder path of each case you report, call `get_repository_folder` with `expands` set to `folders`. Send only the parameters named in this step. Do not add date filters or any other filter. |
There was a problem hiding this comment.
🎨 SUGGESTED: Step C's search fan-out has no cap — an unclear client can reach ~66 calls before any other Testmo request.
Details and fix
This step says to call get_repository_cases "once per client folder and once for New Release", then "one call per distinctive term from the acceptance criteria".
When the client is unclear, line 74 directs the skill to search all client folders plus New Release — 11 folders. With six distinctive AC terms that is 66 search calls before a single get_repository_folder, get_repository_case, or get_case_result_history runs.
Nothing bounds it. Line 183 tells the skill what to do after it has been rate-limited, which means the budget is spent on breadth-first name searches and the detail reads that produce the actual recommendations are the ones that get cut off.
Cap the fan-out in this step:
Use at most five distinctive terms, chosen from the ACs by specificity, and stop searching after 40 calls, recording the unsearched folders under NOT CHECKED.
validate-ai
| allowed-tools: "Read, Write, Grep, Glob, Bash(date:*), Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh api repos/bitwarden/*), Bash(gh search code:*), Bash(git rev-parse:*), Bash(git remote get-url:*), Bash(git -C * rev-parse:*), Bash(git -C * remote get-url:*), Bash(git clone:*), Skill(bitwarden-atlassian-tools:researching-jira-issues)" | ||
| description: Use when determining what test coverage ALREADY exists for a specific change (a PR, Jira key, Tech Breakdown doc, Testmo CSV, changed paths, or named component), or which existing Testmo cases and tests it makes obsolete, wrong, or duplicated, including other teams' cases. Use it before writing new tests or Testmo cases. Triggers on "what's already tested", "does this PR have tests", "what coverage exists for", "is this component covered", "which behaviors have no test today", "which Testmo cases does this story make obsolete", or "which tests should be retired or updated for PM-123". A backward-looking review: it recommends update or retire, lists possible duplicates, notes other teams' affected cases, and never writes to Testmo or code. Do NOT use it to decide which new tests to add ("should I add integration tests here", "are unit tests enough"), to design a test strategy or plan, to run or fix existing tests, or to explain testing concepts like the test pyramid or which layers a repo uses. | ||
| argument-hint: "[PR URL | Jira key | Tech Breakdown doc | Testmo CSV] [designing-test-plans plan path]" | ||
| allowed-tools: "Read, Write, Grep, Glob, Bash(date:*), Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh api repos/bitwarden/*), Bash(gh search code:*), Bash(git rev-parse:*), Bash(git remote get-url:*), Bash(git -C * rev-parse:*), Bash(git -C * remote get-url:*), Bash(git clone:*), Skill(bitwarden-atlassian-tools:researching-jira-issues), mcp__testmo__get_states, mcp__testmo__get_statuses, mcp__testmo__get_repository_folders, mcp__testmo__get_repository_folder, mcp__testmo__get_repository_cases, mcp__testmo__get_repository_case, mcp__testmo__get_case_result_history, mcp__plugin_bitwarden-testing-tools_bitwarden-test-automation__find_automated_cases, mcp__plugin_bitwarden-atlassian-tools_bitwarden-atlassian__get_issue" |
There was a problem hiding this comment.
🎨 SUGGESTED: Bash(git clone:*) is an unrestricted-URL network fetch that undercuts the containment the new disallowed-tools line establishes.
Details and fix
The new line 6 denies Bash(curl *), Bash(wget *), WebFetch, and .env reads, keeping the skill's reads on sanctioned channels. That is the right direction, and the PR deserves credit for it.
Bash(git clone:*) on this line takes an arbitrary URL and is still allowed. No step in the skill uses it, and gotcha line 179 explicitly tells the skill not to clone and to use gh instead.
To be clear on scope: the grant predates this PR, so it is not a widening introduced here. It is flagged because line 6 is new and states an intent this leftover grant defeats — closing it is what makes the new deny list mean what it says.
Drop it from allowed-tools:
allowed-tools: "Read, Write, Grep, Glob, Bash(date:*), Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh api repos/bitwarden/*), Bash(gh search code:*), Bash(git rev-parse:*), Bash(git remote get-url:*), Bash(git -C * rev-parse:*), Bash(git -C * remote get-url:*), Skill(bitwarden-atlassian-tools:researching-jira-issues), mcp__testmo__get_states, mcp__testmo__get_statuses, mcp__testmo__get_repository_folders, mcp__testmo__get_repository_folder, mcp__testmo__get_repository_cases, mcp__testmo__get_repository_case, mcp__testmo__get_case_result_history, mcp__plugin_bitwarden-testing-tools_bitwarden-test-automation__find_automated_cases, mcp__plugin_bitwarden-atlassian-tools_bitwarden-atlassian__get_issue"
validate-ai
|
|
||
| If the user also gives a `designing-test-plans` plan (a file path or pasted content), read it. Keep its acceptance criteria (`AC-1`, `AC-2`, ...), its Tests table (test ID, layer, method), and its Stated scope. Use the AC IDs and plan items as the reasons for every Testmo recommendation. | ||
|
|
||
| If no plan path was given and the input names a Jira story, look in `${CLAUDE_PLUGIN_DATA}/designing-test-plans/` for files named exactly `<KEY>-<YYYY-MM-DD-HHMMSS>-test-plan.md`, where `<KEY>` is that story's key. Ignore every other file, including copies with extra text in the name. If any match, use the one with the latest timestamp in its file name as the plan (not the file's modified time), and name the file and its date in the report's Evidence & sources. If none exist, continue without a plan. |
There was a problem hiding this comment.
🎨 SUGGESTED: The new plan-discovery glob fails silently when ${CLAUDE_PLUGIN_DATA} did not expand, unlike the sibling skill that writes to the same directory.
Details and fix
This step reads ${CLAUDE_PLUGIN_DATA}/designing-test-plans/ with no check that the variable resolved, and its failure mode is "If none exist, continue without a plan" — indistinguishable from a genuinely empty directory.
designing-test-plans/SKILL.md:17 guards the same path explicitly on the write side:
If it still contains
${or starts with/designing-test-plans, the plugin data variable was empty. Stop with a clear error.
The read side having no equivalent matters because the plan drives the recommendations: step E at line 98 refuses any Update or Retire not backed by an AC or plan item. An unresolved path degrades the whole Testmo section while reporting nothing.
Add a guard before the glob:
If the resolved directory still contains `${` or starts with `/designing-test-plans`, the plugin data variable was empty. Record "plan directory unresolved" under NOT CHECKED rather than continuing as if no plan existed.
validate-ai
| 4. Record each behavior: layer (unit / integration / E2E), representative test permalink(s), count, source. Behaviors with no test found → gaps. | ||
| 5. Write the report to `${CLAUDE_PLUGIN_DATA}/coverage-reports/<slug>-<timestamp>-coverage.md` (`<slug>` from the ticket/PR/feature; `<timestamp>` from `date +%Y-%m-%d-%H%M%S`) using the template below. | ||
| 4. Record each behavior: layer (unit / integration / E2E), representative test permalink(s), count, source. Behaviors with no test found are gaps. | ||
| 5. Run the Testmo procedure below for every story. If Testmo cannot be read, record why in the Testmo section and continue with the code side. |
There was a problem hiding this comment.
🎨 SUGGESTED: Step 5 runs the full Testmo procedure for inputs that can produce no recommendation.
Details and fix
This step says to run the Testmo procedure "for every story", but step 1 accepts a PR URL, a Tech Breakdown doc, and a Testmo CSV, and the description at line 3 advertises all four.
For an input that resolves to no Jira story and no plan there are no ACs. Line 98 then forbids every outcome the procedure exists to produce:
Every retire and update names the AC or plan item it rests on. If no AC or plan item supports a recommendation, do not make it.
So steps C through F run their folder searches, case fetches, and history calls at full cost and step E can emit nothing. The report shows "No Testmo cases need changes for this story" (line 228), which reads as a verified negative rather than as "there was nothing to check against".
Add one sentence to this step:
If the input resolves to no story and no plan, skip the Testmo procedure and record "no AC source, Testmo cases not reviewed" under NOT CHECKED.
validate-ai
|
|
||
| 10. Before writing the plan, check that every behavior AC (scope ACs excluded) appears in at least one test row or in the Unspecified table. Add any missing AC to the Unspecified table with "no test derived" in the Source column. Write the plan using the template below to `${CLAUDE_PLUGIN_DATA}/designing-test-plans/<key>-<timestamp>-test-plan.md`. Get `<timestamp>` by running `date +%Y-%m-%d-%H%M%S` as a single command with nothing else in it (no echo, pipes, or command substitution), then use its output. State the full path when done. If the write fails, say so and return the full plan inline instead of a path. | ||
|
|
||
| 11. Hand the saved plan to coverage. Only when the user's request also asks about existing tests or Testmo cases, and only after the plan is saved, invoke `Skill(bitwarden-testing-tools:assessing-test-coverage)` with the key and the saved plan's full path. Pass only those two, never the plan's contents. If the plan write failed, do not invoke it, and tell the user the coverage review needs a saved plan. If the call fails, report it and keep the plan path in your answer. Report the coverage result's report path along with the plan path. |
There was a problem hiding this comment.
🎨 SUGGESTED: Step 11 invokes a sibling skill with no availability check, unlike the two other sub-skill calls in this file.
Details and fix
Lines 25–26 guard both other sub-skill invocations with an explicit "if not available, stop and tell the user to install bitwarden-testing-tools". Step 11 has no equivalent — its only guard is "If the call fails", which is ambiguous when the skill is absent rather than erroring.
The failure mode is mild (the plan is already saved by this point) but the user gets no explanation for why the coverage review they asked for never appeared.
Add the same guard the file already uses, before the invocation:
If `Skill(bitwarden-testing-tools:assessing-test-coverage)` is not available, say so, return the plan path, and stop.
validate-ai
|
|
||
| 10. Before writing the plan, check that every behavior AC (scope ACs excluded) appears in at least one test row or in the Unspecified table. Add any missing AC to the Unspecified table with "no test derived" in the Source column. Write the plan using the template below to `${CLAUDE_PLUGIN_DATA}/designing-test-plans/<key>-<timestamp>-test-plan.md`. Get `<timestamp>` by running `date +%Y-%m-%d-%H%M%S` as a single command with nothing else in it (no echo, pipes, or command substitution), then use its output. State the full path when done. If the write fails, say so and return the full plan inline instead of a path. | ||
|
|
||
| 11. Hand the saved plan to coverage. Only when the user's request also asks about existing tests or Testmo cases, and only after the plan is saved, invoke `Skill(bitwarden-testing-tools:assessing-test-coverage)` with the key and the saved plan's full path. Pass only those two, never the plan's contents. If the plan write failed, do not invoke it, and tell the user the coverage review needs a saved plan. If the call fails, report it and keep the plan path in your answer. Report the coverage result's report path along with the plan path. |
There was a problem hiding this comment.
🎨 SUGGESTED: The saved plan file gets no pointer to the coverage review produced from it, so the link exists only in the chat reply.
Details and fix
Step 11 says to "Report the coverage result's report path along with the plan path", but only in the answer to the user. The output template header (lines 84–90) gained no corresponding field, so the plan written to ${CLAUDE_PLUGIN_DATA}/designing-test-plans/ is a standalone artifact with no reference to the coverage report written in the same run.
Anyone reopening the plan later — which is the point of saving it to a stable path — cannot find the review that went with it.
Add a line to the template header after Manual tests: (line 90), and have step 11 fill it:
Coverage review: <report path>, or not run.
validate-ai
| "declaration": true, | ||
| "declarationMap": true, | ||
| "sourceMap": true | ||
| }, |
There was a problem hiding this comment.
🎨 SUGGESTED: Test-only fixture code is compiled into the production build that ships to every plugin user.
Details and fix
exclude lists src/**/*.spec.ts, but src/github/tar-fixtures.ts is not a .spec.ts file. Its own header says it is not used by the server, and it is imported only by spec files — yet it compiles to build/github/tar-fixtures.js and is shipped by the pnpm run build the launch command runs (plugins/bitwarden-testing-tools/.mcp.json:8, which builds with --prod).
Dead code on the runtime surface of a server that otherwise keeps a tight read-only footprint.
Add the fixture pattern to exclude:
"exclude": ["node_modules", "build", "src/**/*.spec.ts", "src/**/*-fixtures.ts"]validate-ai
…essing-test-coverage (QA-2464)
|
|
||
| I. **Possible duplicates.** A possible duplicate is two cases that test the same behavior, whether or not their names match. Same name is one way to spot one, not the only way. Check every full-review case, and each heads-up case that needs an update or retire under step E, whichever folder it sits in. Report both IDs, both links, and both sets of steps. The team decides. A newer case that covers the same behavior as an older one is a possible duplicate, not a reason to retire the older case. Do not recommend retiring either case on that basis. | ||
|
|
||
| J. **Plain notes.** Say these in the case's entry, without judgment: |
There was a problem hiding this comment.
Details and fix
This is the same class as the already-raised step F finding, on a different step.
Step J requires these in "the case's entry":
- Needs Review or Remove After Release is checked
- The state is not Active
- Automation Type is
Ready to Automate - The Team field is empty, say "No team set"
The Retire layout covers two of them: State: (line 247) and Checkboxes: (line 250). It has no empty-team field.
The Update layout (lines 252-260) has Automation Type: and nothing else from that list: no State:, no Checkboxes:, no empty-team field.
Line 231 then closes off the workaround:
An entry holds only the fields in its layout. No parenthetical remarks, and no comments after an entry.
So for an Update entry, three of step J's four notes are mandatory to produce and impossible to place. Step J is literally unsatisfiable there, and "No team set" is unplaceable in either layout.
Add the missing fields to both layouts, each marked conditional so they stay off a normal entry:
State: <only when not Active, e.g. "Draft, not Active">
Team: <"No team set", only when the Team field is empty>
Checkboxes: <Needs Review / Remove After Release, only when checked>
validate-ai
| const read = new Date(ctx.now()).toISOString(); | ||
| const header = [ | ||
| `Source: ${CODE_REPO}, ${CODE_BRANCH} at ${index.sha} (read ${read}).`, | ||
| `Scanned ${index.filesScanned} .cs files under *.Tests folders. ProdSmoke is excluded on purpose.`, |
There was a problem hiding this comment.
*TestBase.cs, so the skill reports those cases as false integrity mismatches.
Details and fix
src/code/scan.ts:91 drops two classes of file, not one:
if (!path.endsWith(".cs") || path.endsWith("TestBase.cs")) {
return false;
}
return !segments.includes("ProdSmoke");The header this line builds discloses only the second:
Scanned ${index.filesScanned} .cs files under *.Tests folders. ProdSmoke is excluded on purpose.
That matters because the consuming skill converts a non-result into a reported finding. assessing-test-coverage/SKILL.md:147:
The type is
Automated, but the tool finds no match, is a mismatch.
A case whose [Description("N")] lives in a *TestBase.cs therefore lands in the INTEGRITY MISMATCHES table as an automation gap, with nothing in the tool output to tell the reader a whole class of files was never read. The skill is also told at line 150 never to state a case has no test unless the tool returned that result, so it has no way to caveat it.
Name the exclusion in the header:
`Scanned ${index.filesScanned} .cs files under *.Tests folders. ProdSmoke folders and *TestBase.cs files are excluded on purpose.`,validate-ai
| Now: <current step or expected result> | ||
| Proposed: <proposed step or expected result> | ||
|
|
||
| HEADS-UP FOR OTHER TEAMS |
There was a problem hiding this comment.
🎨 SUGGESTED: The HEADS-UP layout omits the folder, the why, and the retire note that line 234 requires on every entry, for the readers least able to supply them.
Details and fix
Line 234 states the rule:
Readers may not have a Testmo seat, so each entry stands on its own: link, folder, why, and the note to paste or the proposed text.
The HEADS-UP layout gives a link, a free-text message, and Confidence. No Folder:, no Why:, and no slot for the note.
The gap bites hardest here because of where line 235 routes work:
A heads-up case that needs a change is listed there, not under SUGGESTED CHANGES.
So a heads-up case needing a retire never appears under SUGGESTED CHANGES, and step E at line 125 still requires a retire note for it. The note is produced with nowhere to go, and line 231 bans appending it as a remark. The other team, who by definition did not run the review and are the people line 234 is written for, get the least complete entry in the report.
Add the three fields to the layout:
HEADS-UP FOR OTHER TEAMS
<Owning team>, ready to forward:
<short message: what changed, and the case to check>
<case ID> <case name>: <link>
Folder: <folder path>
Why: <AC-N or plan item, and what in the case is affected>
Note to paste: <the retire note or proposed text, exactly as step E gives it>
Confidence: <High | Low. Trigger N: <one line quoting the words>>
validate-ai
| Automation: <plan wants automation | plan is manual>. Case is <manual | automated>. | ||
| Outcome: <one of the four outcomes from step H>. | ||
|
|
||
| POSSIBLE DUPLICATES |
There was a problem hiding this comment.
🎨 SUGGESTED: POSSIBLE DUPLICATES hardcodes exactly two folder labels, so the duplicate pairs step I defines mostly cannot be written down.
Details and fix
The layout fixes one row per folder name:
New Release: <case ID>, <link>. Steps: <steps>.
Client folder: <case ID>, <link>. Steps: <steps>.
Step I at line 165 defines the check much more widely:
Check every full-review case, and each heads-up case that needs an update or retire under step E, whichever folder it sits in.
Three shapes the layout cannot express, all of them in scope for step I: two duplicates sitting in the same client folder, a pair where one case is in another team's heads-up folder, and more than two cases testing the same behavior. The layout also presumes which of the pair is the New Release one, which step I does not.
Line 229 ("Use only the sections listed below") and line 231 ("An entry holds only the fields in its layout") mean the model cannot adapt it on the fly, so the likely outcome is a real duplicate going unreported or being forced into the wrong label.
Replace the two fixed rows with one repeatable row that carries the folder as data:
POSSIBLE DUPLICATES
<folder path>: <case ID> <case name>, <link>. Steps: <steps>.
<folder path>: <case ID> <case name>, <link>. Steps: <steps>.
The team decides.
validate-ai
| The Testmo section must be as simple as possible. | ||
|
|
||
| - Show only the sections that have entries. No empty headings, no counts of untouched matches, no "keep" list. | ||
| - If nothing needs action, the whole Testmo section is one line: "No Testmo cases need changes for this story." |
There was a problem hiding this comment.
🎨 SUGGESTED: The collapse-to-one-line rule can suppress a populated INTEGRITY MISMATCHES table and a NOT CHECKED section, turning a partial run into a clean bill of health.
Details and fix
This line says:
If nothing needs action, the whole Testmo section is one line: "No Testmo cases need changes for this story."
"Nothing needs action" reads naturally as "no case needs an update or retire", which is the step E outcome. Two other sections can legitimately have entries in exactly that run:
- Step G at line 133 runs the automation check for every full-review case, "including ones the story does not change, because a mismatch is a finding either way". A run with zero update or retire candidates can still produce integrity mismatches.
- Line 230 permits a NOT CHECKED section whenever something could not be checked, including a Testmo read failure. Step 5 at line 40 and step A both route failures there.
A model following this line literally collapses both away and asserts the story is clean, which is the one output that is actively misleading: a rate-limited or partially-failed run looks identical to a verified-clean one.
Scope the rule to all three sections:
- If no case needs an update or retire, and there are no integrity mismatches and nothing to record as not checked, the whole Testmo section is one line: "No Testmo cases need changes for this story."
validate-ai
|
|
||
| Without a plan, answer only question a, and mention Ready to Automate where seen. | ||
|
|
||
| I. **Possible duplicates.** A possible duplicate is two cases that test the same behavior, whether or not their names match. Same name is one way to spot one, not the only way. Check every full-review case, and each heads-up case that needs an update or retire under step E, whichever folder it sits in. Report both IDs, both links, and both sets of steps. The team decides. A newer case that covers the same behavior as an older one is a possible duplicate, not a reason to retire the older case. Do not recommend retiring either case on that basis. |
There was a problem hiding this comment.
🎨 SUGGESTED: Step I must report "both sets of steps" for a duplicate pair, but step C withheld the fetch that produces them.
Details and fix
This step requires the steps text for both cases in a pair:
Report both IDs, both links, and both sets of steps.
Step C at line 82 decides what was fetched, and deliberately fetches a subset:
Call
get_repository_case... for the cases that look related to the ACs. Do not fetch every case in the results.
A duplicate pair is found by comparing behavior across the full-review set, so one half of a pair routinely sits outside "looks related to the ACs" — and the whole premise of step I is that names need not match, so name-similarity will not have pulled it in either. The steps text is only on the detail record, so for those cases the model either reports a pair with half the required content or quietly drops the pair.
This is the mirror image of the already-raised step G finding, where the wide reading won. Here the narrow fetch rule is correct for cost, so the fix belongs in step I:
Before reporting a candidate pair, call `get_repository_case` for either case whose steps step C did not already fetch. If a fetch fails, report the pair with the steps you have and name the missing one under NOT CHECKED.
validate-ai
🎟️ Tracking
QA-2464. Extends assessing-test-coverage in bitwarden-testing-tools for PDLC Stage 3.
📔 Objective
assessing-test-coverage now reviews the existing Testmo cases for a story and recommends update, retire, or keep for each case the change affects, with the AC or plan item behind it. Given a designing-test-plans plan, it reports planned tests that already exist as cases. It also checks each case's Automation Type against bitwarden/test and lists possible duplicates and cases owned by other teams.
Testmo is read only through Testmo's official MCP server in read-only mode, with setup in the README. A new bundled MCP server, bitwarden-test-automation, adds one tool, find_automated_cases, which finds case IDs in bitwarden/test main through the user's gh login. designing-test-plans now hands its saved plan to assessing-test-coverage when the request also asks about existing tests. assessing-test-coverage runs forked and blocks curl, wget, WebFetch, and .env reads while it runs.
🧪 Testing
Trigger evals with run_real_eval.py, 7 runs per query:
Headless runs on PM-39988 and PM-41861: story review with Testmo connected, a wrong Testmo key, no Testmo server, a plan-only request, a run given a plan path, and readiness, plan, and coverage from one request. With no Testmo server, the skill says it isn't connected, points to the README, and finishes the code side.
MCP server: 57 unit tests pass. The CI install and launch steps pass locally on Node 22. Node 24 and 26 run in this PR's CI.
Notes: