Improvements to H1 Audit skill - #163
Conversation
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
Still hoping to see the prompting for dates when a code change happens. Automation in Jira can be added for some of that to actually prompt someone, but if you are already querying GitHub I would think this can call out what to set a confirmation date to.
| @@ -11,109 +11,153 @@ allowed-tools: mcp__plugin_bitwarden-atlassian-tools_bitwarden-atlassian__search | |||
| | 🔴 | **Update VULN Status** | Child item has progressed (In Progress/Review) but VULN is still at a lower status | | |||
| | 🟡 | **Mark Remediated** | Child item is Done — set Remediation Date to merged PR date and move VULN to Remediated | | |||
| | 🟢 | **Verify & Close** | Fix is in a release that has already shipped — verify in prod, add Confirmation Date, close HackerOne | | |||
There was a problem hiding this comment.
🎨 As we discussed in chat, the confirmation date usually gets set shortly after one is remediated, so you know when the release will go out and therefore the validation.
There was a problem hiding this comment.
I keep trying to figure out a good way to calculate the confirmation date, and haven't come up with anything. We have to take the merge date of a PR, compare it to a release cut date, and then decide which release it will be in accordingly. But getting the release calendar into this skill is a bit of a mess each time I try. This is one of those things that may be easier to handle manually?
Do you have any thoughts on how to handle this better?
There was a problem hiding this comment.
You should just do something like prompt the user to enter the confirmation date.
The audit started from a `project = VULN` JQL sweep, so any open HackerOne report that never got a VULN ticket created was invisible to it. The skill now enumerates open reports across both programs (bitwarden VDP and bitwarden-bbp Bug Bounty) via the HackerOne MCP and correlates outward to VULN tickets, child engineering items, fix PRs, and release state. New action tokens cover the gaps this exposes. 🆕 Create VULN Ticket fires on an open report with no linked VULN, 🟣 Close HackerOne Report on a VULN that is already Closed or Rejected while the report stayed open, and 🏁 Close Out on a Verified VULN whose fix is confirmed in production but whose ticket was never moved to Closed. A reconciliation sweep and 🗂️ Orphaned VULNs section catch open Jira tickets whose report was already closed, which a pure H1-first traversal would otherwise miss. Child-status handling gains a Blocked category so On Hold work reports as stalled instead of collapsing into ⚪ Waiting, plus Ready for Dev and In QA. The query exclusion list widens to (Done, Closed, Rejected, Resolved, Canceled), since VULN-project `status not in (Done, Verified)` does not actually exclude Rejected or Closed. Two correctness fixes on the GitHub side. The PR search `--jq` filter reads `.pull_request.merged_at`, because the Search Issues API returns no `mergedAt` field and the old filter reported every PR as unmerged. Release inclusion now picks the earliest release whose `publishedAt` postdates the merge and confirms it by grepping the tag-to-tag commit range for the PR number, so a missing cherry-pick reports as 🔵 Monitor with an engineering flag rather than counting as shipped. Also documents the API constraints hit in practice (base64 GID report ids, the Jira reference living in get_report_activities, search_issues silently dropping issuelinks, search_reports returning HTTP 500 on the open and needs-more-info states) and adds tool-usage guardrails against interpreter pipes, file writes, heredocs, shell orchestration, and error suppression.
16a034c to
0d13b9e
Compare
Claude Code validationResult: Issues found Validated PR #163 ( CriticalNone. Major
Minor
What passed
Checks run
Two notes on coverage. The changeset touched no |
Narrows the tool grants, fixes four correctness bugs the validator caught, and documents the skill's external dependencies. The `Bash(base64 *)` grant permitted reading an arbitrary file in a skill whose own rules forbid touching the filesystem, so it narrows to `Bash(base64 -d)`, which only matches the stdin decode form the skill actually uses. Unused `Bash(gh search prs *)` and `mcp__hackerone__get_current_user` grants are dropped. Commit-message matching moves out of a `grep` pipe and into the `gh api --jq` expression via `test()`, so no `grep` grant is needed and the pipelines no longer use ungranted commands while the rules claim otherwise. `Bash(echo *)` is added for the base64 input it was always feeding. Cherry-pick verification no longer trusts a truncated commit range. The compare endpoint caps `.commits` at 250 and reports no error when it truncates, which a normal clients release range exceeds (`web-v2025.5.0...web-v2025.6.0` reports 281 and returns 250), so a fix shipped in the tail was reported as a missed cherry-pick and sent engineering after nothing. The check now compares `total_commits` against the returned count, paginates the remainder, and treats an unmatched truncated range as inconclusive. `mergeCommit` returns to the `gh pr view` field list, and that call is now unconditional for direct-push repos. Dropping it left no step returning a commit SHA, so release state for `bitwarden/sm-action` and similar repos could not be determined at all. The Step 2 activity-thread fallback no longer dead-ends when it succeeds. Only the negative case was handled, so a VULN whose description had been reformatted past the tokenizer could be found and then silently dropped, which is the exact miss the traversal order exists to prevent. The reconciliation sweep also stops filing internally-sourced VULNs as orphans with empty HackerOne cells, splitting on whether a report reference exists at all. Consistency fixes: the 🟣 branch names the same five terminal statuses the queries exclude, Blocked is wired into the In Progress branch, the ➖ table row matches the tree, and the Step 5 gate covers a VULN with no children. The Blocked description now states what the skill does rather than claiming a separate category it never routes to. Per review feedback, the prompt-avoidance rationale is gone. Approval-prompt behavior is a property of the run mode a user takes on, not something a skill should encode, and framing the prompt rather than the grant as the failure condition is what produced the ungranted-command bug. The substantive rules stay, minus the reasoning about what does or does not prompt. Also documents the operator-configured HackerOne MCP server and the bitwarden-atlassian-tools dependency in the plugin README, tells the skill what to do when those tools are absent, corrects a platform-specific claim about base64 whitespace handling, trims the description toward the length guideline, and splits the changelog entry across Added/Changed/Fixed.
This is definitely on purpose. This is a stand-alone skill |
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
See the validator comment -- those majors should be resolved. The skill should be broken out to references and probably thinned into several skills; you have a lot of special cases through this and it can't all be in one skill file for this to operate effectively. "Auditing" these is really a large request that needs composition.
Claude loves to make giant changelogs too, so once the above is done make sure that's compressed to meet our norms.
Yea, I'm going to move this back to Draft for now. I was working on it last night, and seeing similar suggestions too. It still needs additional work at this point and shouldn't be poised as ready for review. [EDIT: Doh! It is already in Draft..] |
2 participants
📔 Objective
This inverts the direction of traversal in the
auditing-hackerone-vulnsskill so HackerOne is the source of truth instead of Jira, and folds in a batch of correctness fixes I've been running in my working copy.The audit used to start from a
project = VULNJQL sweep, which meant any open HackerOne report that never got a VULN ticket created was invisible to it. It now enumerates open reports across both programs (bitwardenVDP andbitwarden-bbpBug Bounty) via the HackerOne MCP and correlates outward to VULN tickets, child engineering items, fix PRs, and release state.New action tokens
A reconciliation sweep and a 🗂️ Orphaned VULNs section catch open Jira tickets whose report was already closed, which a pure H1-first traversal would otherwise miss.
Status handling
Child statuses gain a Blocked category, so
On Holdwork reports as stalled rather than collapsing into ⚪ Waiting, plusReady for DevandIn QA. The query exclusion list widens to(Done, Closed, Rejected, Resolved, Canceled), because VULN-projectstatus not in (Done, Verified)does not actually excludeRejectedorClosed.Correctness fixes on the GitHub side
The PR search
--jqfilter now reads.pull_request.merged_at. The Search Issues API returns nomergedAtfield, so the old filter reported every PR as unmerged. Release inclusion picks the earliest release whosepublishedAtpostdates the merge and confirms it by grepping the tag-to-tag commit range for the PR number, so a missing cherry-pick reports as 🔵 Monitor with an engineering flag rather than counting as shipped.Documented API constraints
These are all things I hit in practice and kept re-learning:
get_report_activities, notget_reportsearch_issuessilently drops theissuelinksfield, solinkedIssues()is requiredsearch_reportsrequiresprogram_handlesand returns HTTP 500 for theopenandneeds-more-infostatesFix Version"(Released)" annotation is untrustworthyAlso adds tool-usage guardrails against interpreter pipes, file writes, heredocs, shell orchestration, and
2>/dev/nullerror suppression.Note on history
I force-pushed over the previous two commits on this branch. They had bumped the plugin to 1.3.0, which main then released independently as an unrelated ADR-alignment change, so the branch was conflicting. This version sits at 1.4.0 on top of current main and the changelog is sequential again.