Skip to content

docs: add the security-advisory skill, and reconcile the two rules it contradicts - #2444

Open
cliffhall wants to merge 6 commits into
v2/mainfrom
v2/docs/2443-security-advisory-skill
Open

cliffhall wants to merge 6 commits into
v2/mainfrom
v2/docs/2443-security-advisory-skill

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #2443

A privately reported vulnerability arrives through GitHub's security advisory flow (SECURITY.md routes every report there, and the issue chooser deliberately has no security template). None of that procedure was written down, and sixteen advisories currently sit in triage with hand-made draft cards on board #28.

What this adds

.claude/skills/security-advisory/SKILL.md — the flow end to end:

  1. Advisory lands in triagedraft card on board Add tab and approval flow for server -> client sampling #28, titled [GHSA-xxxx-yyyy-zzzz] - <summary>, body opening with **Advisory:** <html_url>, Status Incoming plus a Priority.
  2. Verify the claim — including who owns the code path. CIMD loopback HTTPS exemption is host-string-based, not resolution-aware #2409 is the worked precedent: a loopback/HTTPS-exemption finding that read as an Inspector defect and lived in @modelcontextprotocol/client (typescript-sdk#2591). The reporter withdrew it. Ownership is checked before severity, not after.
  3. Valid → accept (triagedraft). Invalid or upstream → close with a reason.
  4. Accepted → private fork, fix and review there.
  5. Merge, release, then publish.
  6. After the release, file the public (closed) issue and convert the card.

Two steps are marked human-gated and the skill says so twice: accepting (outward-facing, the reporter sees it) and publishing (irreversible, assigns a CVE, credits the reporter). The skill recommends; a maintainer clicks.

API facts recorded so nobody re-derives them, all verified against the live API:

  • POST …/security-advisories/{ghsa_id}/forks202, creating a private repo <repo>-<ghsa-id> in the org.
  • ⚠️ Read .private_fork first — the POST creates a fork, it does not probe for one. This was learned the hard way by treating it as a probe, and it is asymmetric: deleting a fork needs the delete_repo OAuth scope a default gh token lacks.
  • There is no comment API for advisories — not in REST, not in GraphQL (RepositoryAdvisory is not commentable). Reporter coordination is UI-only.
  • Acceptance is readable as submission.accepted alongside state; states seen are triage / draft / published / closed.
  • The board step cannot be automated: no PROJECT_TOKEN exists in this org and GITHUB_TOKEN structurally cannot hold organization projects: write. The skill says so explicitly, because a nightly workflow is the obvious-looking wrong answer.

The two rule changes that make it coherent

AGENTS.md — "Every board item is a real GitHub issue. No draft cards." was absolute, and the sixteen advisory cards violate it by design: a real issue would disclose the vulnerability before a fix exists. Advisory drafts are carved out as the one exception, with the [GHSA- prefix called out as load-bearing rather than cosmetic. Plus the skills-index row and a refresh of the listing-budget figures (3,2343,679 of 4,000; nine-of-ten → ten-of-eleven model-invoked).

.claude/skills/issue-triage/SKILL.md — the board audit's non-Issue on a board check counted every draft, so it reported 16 today and would have stayed permanently non-zero. A check that never prints 0 stops being read, which is the failure this changes.

The carve-out is by title prefix only, deliberately narrow:

| select(.content.type!="Issue")
| (.content.title // "(untitled)")
| select(startswith("[GHSA-") | not)

Exempting all drafts, or every Incoming card, would let an ordinary stray draft through — the defect the check exists for. Verified both ways:

  • Against the live boards, the amended audit prints 0 for that check with all 16 advisory drafts present (every other check unchanged; the one pre-existing non-zero is #2430's missing labels, untouched here).
  • Against a synthetic item list, a draft titled asdasd and a stray PR are both still reported.

It also now reports the title rather than .content.number, which is null for a draft — the old output could not name what it had found.

Eval cases

.claude/skills/security-advisory/evals/evals.json — six positives and two negatives, per the AGENTS.md floor. Each positive is first-move shaped ("what do I do with it", "how do I reply to", "when is it safe to"), names no concrete file or symbol, and asks for something AGENTS.md does not answer: the rules file now states the draft-card exception, so no case is aimed at that.

npm run skills:eval was not run — it spends metered model calls and is deliberately outside the gate. Worth a run before anyone edits a description here, since adding an eleventh model-invoked skill costs the attention of the ten already there.

Not done, deliberately

No workflow that boards advisory cards automatically. That path is closed by the token constraint above, and the skill records why so it is not rediscovered.

🤖 Generated with Claude Code

…f the board audit (#2443)

A privately reported vulnerability arrives through GitHub's advisory flow, never
as an issue, and none of that procedure was written down. Sixteen advisories sit
in `triage` today with hand-made draft cards on board #28.

- New `security-advisory` skill: the draft card and its `[GHSA-` title prefix;
  verifying WHO OWNS THE CODE PATH before assessing severity (#2409 was a
  loopback/HTTPS finding that lived in `@modelcontextprotocol/client`, not here,
  and was withdrawn); accepting vs closing; the private fork; publishing; the
  public issue afterwards. Accepting and publishing are marked human-gated —
  both are outward-facing and publishing is irreversible.
- Records the API facts that are easy to get wrong: the private-fork POST
  CREATES a fork rather than probing for one (read `.private_fork` first),
  there is no comment API for advisories in REST or GraphQL so reporter
  coordination is UI-only, deleting a private fork needs `delete_repo`, and the
  board step cannot be automated — no `PROJECT_TOKEN` exists in this org and
  `GITHUB_TOKEN` cannot hold `organization projects: write`.
- AGENTS.md: carve advisory drafts out of "no draft cards" as the single
  exception, and add the skill to the index. Refresh the listing-budget figure.
- issue-triage: the board audit's `non-Issue on a board` check counted all 16
  drafts and would have stayed permanently non-zero. It now excludes drafts by
  TITLE PREFIX only, so a stray draft is still reported — and by title, since a
  draft has no issue number to print.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Sep 19, 2026
@cliffhall
cliffhall requested a balanced review from Copilot September 19, 2026 01:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The workflow contains unsafe or impossible handoffs, while the audit can overlook malformed advisory cards.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 6 Medium severity · 2 Low severity

Open (9)
What changed in this PR

Adds a security-advisory workflow and reconciles advisory draft cards with project rules.

Changes:

  • Documents private advisory handling and human approval gates.
  • Adds skill invocation evals.
  • Updates board-audit handling for advisory drafts.
File Description
AGENTS.md Adds the advisory exception and skill metadata.
.claude/​skills/​security-advisory/​SKILL.md Defines the end-to-end advisory workflow.
.claude/​skills/​security-advisory/​evals/​evals.json Adds positive and negative invocation cases.
.claude/​skills/​issue-triage/​SKILL.md Exempts advisory drafts from the non-Issue audit.

Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread .claude/skills/issue-triage/SKILL.md Outdated
Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread .claude/skills/security-advisory/SKILL.md
Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread AGENTS.md Outdated
Seven of nine findings were defects in what this PR added.

- Upstream advisories: "close with a pointer to the upstream issue" would have
  disclosed an unfixed vulnerability on someone else's behalf. Route it through
  the upstream's own private channel; reference a public issue only after they
  publish.
- Step 6 was impossible as written: GitHub's "Convert to issue" creates a NEW
  issue from the draft and cannot bind a card to one filed separately. Convert
  first, then label/milestone/close/move, and skip /issue-create's add-card step.
- Closing a rejected or upstream advisory now says to DELETE its draft card —
  nothing shipped, and the audit's non-Issue check no longer looks at it.
- Drop the /pr-flow pointer from Related and say why: it requires a public
  issue and a public PR, which is the disclosure this flow delays.
- `--paginate` on the triage-advisory listing; the endpoint pages at 30 and a
  truncated inventory reads as "nothing pending".
- Record the Priority arithmetic in the draft body — a draft card has no
  comments, so /issue-triage's "record the score in a comment" cannot be met.
- The exemption needed a replacement check. $B28/$B11 are Issue-only, so the
  non-Issue check was the ONLY one seeing a draft; exempting it alone made a
  half-made advisory card invisible to the whole audit. Added
  `GHSA draft missing Status/Priority`, verified against a synthetic set.
- board-ops and issue-create both still said draft cards are never allowed,
  contradicting the new AGENTS.md carve-out. Both now name the exception.

Declined: chain eval cases for the /board-ops, /issue-triage and /issue-create
pointers. docs/skill-authoring.md puts a chain case in the TARGET skill's file
and requires it be measured, and skills:eval is deliberately outside the gate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

cliffhall commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

Copilot review round 1 — all nine answered (41efd17c)

Replied inline per comment; this is the mirror, since inline replies go hidden once the fix is pushed. No "Suppressed comments" block in this round.

Accepted — 7. All seven were defects in what this PR added, not pre-existing behavior:

Finding Fix
Upstream advisories routed to a public upstream issue Route through the upstream's own private channel; a public issue only after they publish. #2409 re-described as the benign case, not the template
Step 6 was impossible — "Convert to issue" makes a new issue and cannot bind a card to one filed separately Convert first, then label/milestone/close/move; explicit "do not run /issue-create's add-card step"
A closed advisory's draft card left on the board forever Delete it at close time — nothing shipped, and the amended audit no longer reports it
/pr-flow pointer led into a public issue + public PR Removed, replaced by a warning saying why; /issue-create qualified as post-publication
Advisory listing silently truncated at 30 --paginate, with a note that a truncated pending inventory reads as "nothing pending"
Priority recorded with no arithmetic (a draft has no comments) Axes, bonuses and total go in the draft body
board-ops and issue-create still said draft cards are never allowed Both now name the exception; issue-create closes it off explicitly

The one I want to highlight is the audit finding, because it was a blind spot this PR would have introduced. $B28/$B11 are built from Issue items only, so before this change the non-Issue check was the only thing in the audit that ever saw a draft. Exempting drafts there alone would have made a half-made advisory card invisible to every check. Rather than widen $B28 — which would then feed the milestone and version-label checks a card that can hold neither — the audit carries a narrow replacement, GHSA draft missing Status/Priority. Verified both directions: 0 against the live board with all 16 drafts present, and on a synthetic three-card set it reports exactly the two missing a field.

Declined — 1: chain eval cases for the /board-ops, /issue-triage and /issue-create pointers. docs/skill-authoring.md puts a chain case in the target skill's eval file and sets two conditions this PR cannot meet: a chain case is a measured claim scored against CHAIN_THRESHOLD, and skills:eval is deliberately outside the gate because it spends metered model calls — so committing them here ships three unmeasured assertions in files this PR does not otherwise touch. It also only pays where the prompt names nothing about the target, and "add a card" / "file the issue" reach /board-ops and /issue-create as first-move cases. That page separately warns that an eleventh model-invoked skill costs the attention of the ten already there, and only a full suite run observes it — so the right shape is one deliberate RUNS=5 full-suite run covering displacement and any new chain cases, on its own issue.

Gate: npm run local:gate green on 41efd17c (EXIT=0), including verify:skills at 3,679/4,000 listing characters.

On further rounds: this is a docs-only PR and the remaining finding classes are closed — chain evals for the reason above, and anything asking for the Actions automation, which organization projects: write makes structurally unreachable for GITHUB_TOKEN and which this repo has no PROJECT_TOKEN for. New findings about the procedure this PR documents are still welcome.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The audit can miss invalid GHSA-prefixed items, and parts of the procedural guidance remain contradictory or unusable for draft cards.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 Low severity

Open (3)
Resolved since last review (8)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Restrict GHSA exception to board #28 advisory drafts

.claude/​skills/​issue-triage/​SKILL.md:244

This exempts every [GHSA--prefixed non-Issue on both boards, not just an advisory draft on board #28. The replacement check only scans board #28 drafts, so a GHSA-titled PR or a draft accidentally placed on board #11 makes both checks report zero. Restrict the exception to DraftIssue items on #28 and continue reporting all non-Issue items on #11.

Comment thread .claude/skills/board-ops/SKILL.md Outdated
Comment thread .claude/skills/security-advisory/SKILL.md Outdated
All three findings were defects in what this PR added.

- The audit's carve-out matched on the title prefix alone, across BOTH boards
  and ANY non-Issue type. So a pull request titled `[GHSA-…]` — a plausible
  title for a security fix — was exempted, and so was an advisory draft
  misfiled on #11, where the replacement field check does not look either, so
  both checks would have read 0. The exemption is now `DraftIssue` AND the
  `[GHSA-` prefix AND board #28; #11 reports every non-Issue item it carries.
  Verified on a synthetic set: the exempt draft passes, while a stray draft, a
  GHSA-titled PR and a #11 advisory draft are all still reported.
- board-ops claimed every recipe applied to an advisory draft unchanged. It
  does not: a draft has no repository and no issue number, so every
  `select(.content.repository==… and .content.number==…)` matches nothing and
  `item-add --url` has no URL. Added the title-based item-id lookup, keyed on
  the bracketed GHSA id rather than on words from the free-text summary.
- The step table still said step 6 files the public issue and then converts the
  card — the impossible ordering the body below it was fixed to reject. The row
  now names conversion as what creates the issue.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

cliffhall commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

Copilot review round 2 — all three answered (2bba258f)

Two inline comments plus one Previously missed finding, which has no thread to reply into — so this mirror is the only place it can be answered, and it is the most important of the three. All three accepted; all three were defects in what this PR added.

The previously-missed one: the carve-out was matching on a title, not on a card

.claude/skills/issue-triage/SKILL.md:244 — the exemption was select(startswith("[GHSA-") | not) applied to every non-Issue item on both boards. A title is neither a type nor a board, so that exempted two things it should not:

  • a pull request titled [GHSA-…] — a thoroughly plausible title for a security fix, which is what makes this worse than hypothetical;
  • an advisory draft misfiled on get tools working #11, where the new GHSA draft missing Status/Priority check does not look either — so both checks would have read 0 on a card in the wrong place.

The exemption is now three conditions: DraftIssue and the [GHSA- prefix and board #28. Board #11 reports every non-Issue item it carries, as before.

Verified on a synthetic set covering exactly those cases:

Item Reported?
[GHSA-aaaa-…] draft on #28 no — the intended exemption
asdasd draft on #28 yes
[GHSA-dddd-…] fix the thing PR on #28 yes
[GHSA-gggg-…] draft on #11 yes

Live boards still print 0 with all 16 advisory drafts present.

board-ops — "every recipe applies unchanged" was wrong

A draft has no .content.repository and no .content.number, so every select(.content.repository==… and .content.number==…) in that file matches nothing against one, and item-add --url has no URL to be given — which made the delete recipe the rejection step points at unusable. Added the title-based item-id lookup, keyed on the bracketed GHSA id rather than words from the free-text summary.

The step table still described the impossible ordering

Row 6 said "file the public (closed) issue and convert the card" — the ordering round 1 fixed in the body below it, left standing in the summary a reader actually skims. It now names conversion as the operation that creates the issue.

Gate: npm run local:gate green on 2bba258f (EXIT=0).

Closing the loop here

Two rounds, twelve findings, all twelve answered and eleven applied. This is a docs-only PR and the open classes are now closed:

  • chain eval cases — declined in round 1 with reasons: they belong in the target skills' files, they are a measured claim skills:eval cannot make inside the gate, and the prompts that reach /board-ops and /issue-create here are first-move cases by docs/skill-authoring.md's own test. A full RUNS=5 suite run also has to cover displacement onto the ten existing skills, which no focused run can see — that is its own issue, not a file edit in this PR.
  • the Actions automation — structurally unreachable: organization projects: write is a permission GITHUB_TOKEN cannot hold and this org has no PROJECT_TOKEN. The skill records that so it is not re-proposed.

Not requesting further reviews. A finding about the procedure this PR documents is still worth raising on the issue.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The procedure mishandles v1 advisories, misstates CVE publication behavior, and misses advisory drafts with an incorrect status.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Select all affected release branches, not only v2

.claude/​skills/​security-advisory/​SKILL.md:161

This hard-codes the fix to v2/main, but SECURITY.md:7-20 explicitly supports v1 security fixes and asks reporters whether v1, v2, or both. A v1-only advisory would be merged into the wrong branch, while an advisory affecting both could leave v1 unpatched. Select all affected release branches here.

This issue also appears on line 187 of the same file.

Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Both findings were defects in what this PR added.

- The skill assumed v2 throughout: the fix "comes back to `v2/main`", and step
  6 applied the `v2` label unconditionally. But an advisory is very nearly the
  ONLY work the v1 line ever receives — SECURITY.md supports v1 for security
  fixes only, published under `v1-latest` — so the skill was wrong exactly
  where v1 matters most. A v1-only advisory would have been merged to a branch
  where the bug does not exist; one affecting both lines could have been
  published with v1 still unpatched, which is the worst outcome this flow can
  produce.
  Step 2 now ends in a SET of affected lines, with the per-line branch and
  dist-tag table and the note that the two publish independently so a v1 fix is
  not forward-ported. Step 4 merges to each affected branch; step 5 states that
  "shipped" means shipped on every affected line before publishing. Step 6's
  labels, milestone and board branch by line — `v1` takes no milestone (every
  milestone is a v2 release bucket) and is carded on #11, which has no Priority
  field.
  The reporter's v2/v1/both answer is described as what SECURITY.md actually is
  — a request in "What to Include", not a required form field — so it is read
  and then verified rather than trusted.
- The frontmatter description and the Related line still said to "file" the
  public issue, contradicting step 6, which round 1 established must CONVERT
  the existing draft card. Both now say convert. Listing budget re-checked:
  3,711/4,000, and the recorded figure in AGENTS.md moved with it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 3 — both findings answered (b2d44fff)

Both accepted. One has a thread; the other is a Previously missed finding with no thread, so this is the only place it can be answered — and it is the more serious of the two by a distance.

Finding 1 (no thread): the skill assumed v2 everywhere, which is backwards

SKILL.md:161 said the fix "comes back to v2/main", and step 6 applied the v2 label unconditionally. That is wrong in the one place it can do the most harm.

An advisory is very nearly the only work the v1 line ever receives. SECURITY.md supports v1 for security fixes only, published under the v1-latest dist-tag, with no active development otherwise — so "assume v2" is a default that holds for almost all work in this repo and fails for almost all of this work. Two concrete failures it permitted:

  • a v1-only advisory merged to v2/main, a branch where the bug does not exist;
  • an advisory affecting both lines published with v1 still unpatched — a live vulnerability disclosed to the users least able to move, since v1 is the deprecated line people are on precisely because upgrading is hard.

Step 2 now ends in a set of affected lines rather than an assumption, and each ships on its own terms:

Line Branch Flow Publishes to
v2 v2/main fix branch → v2/main → (milestone) main latest
v1 v1/main flat — no merge into main v1-latest

The two publish independently under separate dist-tags, so a v1 fix is not forward-ported; if v2 is affected too that is a second fix on v2/main. Step 4 merges to each affected branch. Step 5 now states that "shipped" means shipped on every affected line before the advisory is published. Step 6's labels, milestone and board branch accordingly — a v1 issue takes no milestone (every milestone here is a v2 release bucket) and is carded on #11, which has no Priority field, so the v1 advisory's #28 draft is deleted rather than left behind claiming v2 work.

One correction to how the reporter's answer is described: SECURITY.md asks for "whether it affects v2, v1, or both" under What to Include — that is a request in prose, not a required form field. The skill says so, so the answer is read and then verified rather than trusted, and its absence is expected rather than surprising.

Finding 2 (thread discussion_r4053211653): "file" vs "convert"

The frontmatter description and the Related line still said to file the public issue, contradicting step 6, which round 1 established must convert the existing draft card — filing separately produces a duplicate issue and a duplicate card. The frontmatter is the sharp case: it is what the model matches on and carries into the task, so it was orienting a reader toward the invalid sequence before the body could correct it. All three sites now say convert.

Listing budget re-checked after the description edit: 3,711/4,000, with the figure recorded in AGENTS.md moved to match.

Gate: npm run local:gate green on b2d44fff (EXIT=0).

Where the loop stands

Three rounds, fourteen findings: thirteen applied, one declined. Requesting round 4 — the stop condition is two consecutive zero-finding rounds, and no round yet has come back empty.

The one declined finding stays declined, for the reasons given in round 1: chain eval cases belong in the target skills' files, they are a measured claim that skills:eval cannot make inside the gate, and the prompts that would reach /board-ops and /issue-create here are first-move cases by docs/skill-authoring.md's own test. The full RUNS=5 suite run that would measure them also has to cover displacement onto the ten existing skills, which no focused run can see — that is its own issue, not a file edit in this PR. Likewise closed: the Actions automation, which organization projects: write makes structurally unreachable for GITHUB_TOKEN in an org with no PROJECT_TOKEN.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The documented flow risks premature disclosure and leaves CVE, multi-release, and card-conversion behavior inaccurate or incomplete.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Add required board transitions during private advisory remediation

.claude/​skills/​security-advisory/​SKILL.md:190

The flow leaves the advisory card in Incoming while the private fix and review happen. That conflicts with the repository-wide lifecycle in AGENTS.md: work beginning moves a card to In Progress, and an open PR moves it to In Review. Add those transitions here so the private board reflects the advisory's actual state instead of jumping directly from Incoming to Done.

Low severity Correct inaccurate CVE and reporter credit publishing claims

.claude/​skills/​security-advisory/​SKILL.md:19

Publishing does not itself assign a CVE or automatically credit the reporter. GitHub documents CVE requests as optional, and credits appear only when someone is explicitly added and accepts. Keeping these as claimed side effects makes the human-gate rationale factually inaccurate.

This issue also appears on line 206 of the same file.

Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Four accepted, one accepted in part.

- The card body no longer carries the vulnerability description. Project access
  and advisory access are SEPARATE permission sets, so the board's audience is
  not the advisory's audience. The boards are private, so this is a wider
  audience than intended rather than a public leak — but a repro or PoC belongs
  with the people handling it. Link plus triage metadata only.
- Publishing does NOT assign a CVE or credit the reporter. A CVE request is
  optional and a credit must be explicitly added and then accepted. Both are
  now stated as things to do BEFORE publishing, which is more useful than the
  overclaim was: an unadded reporter is simply never credited and nothing
  reports it.
- The card now moves In Progress / In Review with the work. It being private is
  the reason to do this, not to skip it — the fork is invisible to anyone not
  on the advisory, so the card is the only signal the work exists.
- The "both lines" case gets a deterministic sequence: the draft converts
  exactly once, so v2 inherits the conversion on #28 and v1 is filed separately
  on #11. A v1-ONLY advisory deletes the #28 draft rather than converting it,
  since a converted card would put a v1 issue on #28.
- The `/release` pointer added in round 3 is removed. It was a dead end twice
  over: `release` is `disable-model-invocation: true`, which AGENTS.md says a
  skill cannot reach, and it moves through two PUBLIC PRs.

Declined the second half of that last one — designing a security-release path
that publishes without the public-PR sequence. The premise does not hold: the
patch stops being secret at MERGE, since merging the private fork puts an
ordinary public commit on the release branch, so no release path can keep it
private. Stated that directly instead, with the actionable consequence: the
merge-to-publish window is exposure, not secrecy, so merge close to the release.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 4 — all five answered (4da164ac)

Three inline comments plus two Previously missed findings, which have no threads — so this mirror is the only place those two can be answered. Four accepted outright, one accepted in part.

Previously missed (no thread): publishing does not assign a CVE or credit the reporter

SKILL.md:19 and :206 both claimed it does. That was factually wrong: requesting a CVE is an optional action on the advisory, and a credit appears only when someone is explicitly added and then accepts it.

The fix is not just to delete the overclaim, because the underlying point was worth keeping — it was simply attached to the wrong step. Both are now stated as things to do before publishing, which is more useful than the original: an unadded reporter is never credited, and nothing reports that. It is a silent omission against the one person who did us a favour.

Previously missed (no thread): the card never moved

The flow left the card in Incoming from arrival until it jumped to Done, contradicting AGENTS.md's lifecycle. The card now moves In Progress when the fix starts and In Review when the fork's PR is open.

Worth stating why the private case is the strong case rather than an exception: the fork is invisible to everyone not on the advisory, so this card is the only place the rest of the team can see the work exists. Leaving it in Incoming reports "unreviewed, nobody has committed to this" for the entire time somebody is actively fixing it.

The card body carried the full vulnerability description

Project access and advisory access are separate permission sets, so the board's audience is not the advisory's audience. One precision I added rather than leaving implicit: the boards are private, so this is a wider audience than intended, not a public leak — which does not rescue the instruction, but does matter for urgency. Card now carries link plus triage metadata only.

⚠️ The sixteen advisory cards already on #28 predate this PR and do carry full descriptions. This PR deliberately does not touch them — its scope is the procedure — but that is a real cleanup for a maintainer to decide on, and I would rather flag it than let the instruction change read as though it had fixed it.

The "both lines" case had no determinate procedure

There is only one draft card and it converts once, while each issue needs exactly one version label and one board — so "one issue per line" was hand-waving. Now: convert the draft into the v2 issue on #28 (it already lives there, and v2 is the line with a milestone to record); file the v1 issue separately via /issue-create on #11 with no milestone and Status only; cross-link, close both, move both to Done.

That also surfaced a case nobody had written down: a v1-only advisory. Its draft sits on #28, the wrong board, and converting cannot move it — a converted card would put a v1 issue on #28, which the audit reports as a wrong-board card. So that path files on #11 and deletes the #28 draft.

Accepted in part: the /release pointer

Accepted — removed. I verified both halves of the finding and both hold: release is disable-model-invocation: true, which AGENTS.md says a skill cannot reach from another skill's body, so the pointer I added in round 3 was a dead end on top of everything else; and its own description confirms two public PRs.

Declined — defining a security-release path that publishes without the public-PR sequence. I checked the premise before building on it, and it does not hold. The patch stops being secret at MERGE, not during the release: merging the private fork puts an ordinary public commit on v2/main or v1/main, which is just how a private fork lands, before any release procedure starts. A bespoke release path would therefore protect nothing still protected by the time it ran.

The skill now says the true and actionable thing instead: the merge-to-publish window is a window of exposure to anyone reading commits, not of secrecy, so it should be short — merge close to the release, publish as soon as it is out. Inventing a second release procedure would also be a maintainer's call about how this project ships, not a docs change inside a PR documenting the existing flow.

Gate: npm run local:gate green on 4da164ac (EXIT=0). Listing budget unchanged at 3,711/4,000.

Loop status

Four rounds, nineteen findings: seventeen applied, two declined with reasons (chain evals in round 1; the bespoke security-release path here). Requesting round 5, which is the cap — if it returns findings I will answer them and stop there rather than continue, and say so.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The procedure still contains contradictory priority ordering, status/milestone rules, and release-line-specific conversion instructions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Low severity Priority is assessed before ownership despite required ordering

.claude/​skills/​security-advisory/​SKILL.md:76

Step 1 requires a rubric-derived Priority before step 2 establishes ownership, but the rubric's first axis is severity and line 104 explicitly requires ownership to be checked before severity. This forces the assessment into the exact order the worked precedent warns against. Reorder these steps, or explicitly make the initial score provisional and state when it is rescored.

This issue also appears in the following locations of the same file:

  • line 209
  • line 264

Comment thread .claude/skills/security-advisory/SKILL.md Outdated
Comment thread AGENTS.md Outdated
All three findings were internal contradictions in text this PR added.

- Step 1 required a rubric Priority, whose FIRST axis is severity, while step 2
  says ownership is established before severity — so the procedure forced the
  exact order #2409 is the warning against. The step-1 score is now explicitly
  PROVISIONAL, recorded as such and dated in the draft body, and re-scored at
  the end of step 2 when ownership and the affected lines are known. The
  provisional line stays when re-scored so the change of view is legible.
- The flow table, the frontmatter description and the Related line all made
  conversion universal and triggered on "after the release". Both were wrong:
  a v1-only advisory FILES on #11 and deletes the draft, and the trigger is
  PUBLICATION, since the release ships the fix while the advisory can still be
  private — a public issue opened in that gap describes an undisclosed
  vulnerability. All three sites are now per-line and post-publication, and
  step 6 states the reason rather than just the rule.
- AGENTS.md's carve-out said the draft is always converted once published,
  contradicting the per-line procedure. It now states the per-line outcome.

Listing budget re-checked after the description edit: 3,729/4,000, with the
recorded figure moved to match.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 5 — all three answered (856c82f5), and the loop stops here

Two inline comments plus one Previously missed finding, which has no thread — so this mirror is the only place it can be answered, and it is again the most interesting of the three. All three accepted. Every one was an internal contradiction in text this PR added.

Previously missed (no thread): the procedure forced the exact ordering it warns against

SKILL.md:76 (and :209, :264). Step 1 required a Priority scored with the /issue-triage rubric. That rubric's first axis is severity. Step 2 says — emphatically, with #2409 as the worked precedent — that ownership is established before severity, because a report can look severe right up until the code turns out not to be ours.

So the two steps, each defensible alone, combined into an instruction to assess severity before ownership: the precise failure the skill spends a paragraph warning about, embedded in its own step 1.

The fix is not to reorder the steps, because the card genuinely has to exist before the investigation — it is what records that the investigation is happening. Instead the step-1 score is now explicitly provisional:

  • score what the report claims, mark it provisional and dated in the draft body;
  • re-score at the end of step 2, which is the first point at which the severity axis has anything solid under it — the code is confirmed ours, it has been reproduced, and the affected lines are known ("affects both lines" being itself a severity input the provisional score could not have had);
  • leave the provisional line in place and add the new one beneath it, so the change of view is legible rather than overwritten;
  • an advisory that turns out to be upstream has its card deleted, not re-scored.

"Provisional" here is not a hedge to make a contradiction go away — at step 1 you have a report and nothing verified, so it is the only honest label for that number.

Conversion was universal, and triggered on the wrong event

The flow table, the frontmatter description and the Related line all still said "convert the draft card, after the release". Two errors:

  • "Convert" is per-line since round 4 — a v1-only advisory files on get tools working #11 and deletes the Add tab and approval flow for server -> client sampling #28 draft; a both-lines advisory files a separate v1 issue. The summary was describing the v2 path as if it were the rule.
  • "After the release" is the wrong trigger. The release ships the fix; the advisory can still be private at that moment. A public issue opened in that gap describes an undisclosed vulnerability — the failure this whole skill exists to prevent, reintroduced in its own last step.

All three sites are now per-line and post-publication, and step 6 opens by stating why rather than just the rule, so a later edit cannot quietly relax it back.

AGENTS.md contradicted the procedure

Same defect, in the file where it matters most: AGENTS.md is resident on every turn, so when it and a skill body disagree, the rules file is what a model has already read. It now carries the per-line outcome and the publication trigger, with the reason the v1 path cannot be a conversion — a draft cannot convert onto another board, and a v1 issue on #28 is a wrong-board card the audit reports.

Gate: npm run local:gate green on 856c82f5 (EXIT=0). Listing budget 3,729/4,000 after the description edit, with the recorded figure in AGENTS.md moved to match.


Stopping here — five rounds, 22 findings, 20 applied

Round Findings Applied Declined
1 9 8 1 — chain eval cases
2 3 3
3 2 2
4 5 4 + 1 in part the bespoke security-release path
5 3 3

This is the agreed cap and no further reviews will be requested. The review has been valuable well past where I expected — rounds 4 and 5 each found a genuine contradiction — but the two open classes are closed on reasons that another round cannot change:

  • Chain eval cases belong in the target skills' files, assert a measured rate that skills:eval cannot produce inside the gate, and would cover prompts that reach /board-ops and /issue-create as first-move cases anyway. The RUNS=5 full-suite run that would measure them also has to observe displacement onto the ten existing skills — that is its own issue, not a file edit here.
  • A security-release path that publishes without public PRs rests on a premise that does not hold: the patch stops being secret at merge, since merging the private fork puts an ordinary public commit on the release branch before any release procedure begins.

Two follow-ups for a maintainer, neither in scope for this PR and both surfaced by the review rather than hidden by it:

  1. The sixteen existing advisory cards on Add tab and approval flow for server -> client sampling #28 carry full vulnerability descriptions. The instruction is fixed; those cards were created before this PR and are deliberately untouched. Whether to trim them is a call about live data, and the boards being private bounds the exposure but does not remove it.
  2. The eval suite has not been run. It spends metered model calls and is outside the gate by design; adding an eleventh model-invoked skill is exactly the change that needs a full RUNS=5 run to see what it costs the other ten.

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

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants