* 🔒 Fix once-per-client promo race in checkout
Re-check the once-per-client promo limit inside the checkout transaction
under a client-row lock, so concurrent checkouts for the same client
serialize instead of all passing the pre-transaction check. Losing
checkouts fail with the same error as a normal reuse.
* Clarify advisory sibling references
* 🔒 Skip the aggregate lock on PostgreSQL in the promo guard
PostgreSQL rejects FOR UPDATE on aggregate queries, so the locking
COUNT fails there with a database error. The client-row mutex stays
locking on every driver; the COUNT goes plain on PostgreSQL, which
loses nothing under READ COMMITTED once the mutex is held. Adds mock
and live-PostgreSQL coverage for both shapes.
* 🔒 Skip the aggregate lock on PostgreSQL in the balance guard
The same PostgreSQL restriction found in review applies to the
pre-existing client-balance locking read: SUM with FOR UPDATE fails
there, breaking every credit payment on PostgreSQL installs. Same
treatment as the promo guard: mutex stays locking everywhere, the
aggregate goes plain on PostgreSQL, with mock and live-PostgreSQL
coverage.
* Add security-advisory-fix skill
Companion to security-advisory-triage: that skill verifies a reported
advisory and writes it up, this one covers building, testing, and
shipping the actual fix once an advisory is confirmed real. Codifies
what came out of doing this end to end on a real advisory this
project handled recently:
- Designing the fix as a real decision, not a foregone conclusion:
looking for an already-solved pattern elsewhere in the app first,
laying out genuinely distinct strategy options for AskUserQuestion
rather than picking silently, and checking whether a fix closes the
underlying mechanism or just the reported instance of it (with
guidance broken out by bug class: disclosure/enumeration oracles,
validation/injection/traversal gaps, and broken access control).
- What to do when exploring the code contradicts what the report
claims, in either direction: something adjacent and real turns out
to be the actual issue, or nothing is actually wrong and the report
is a false positive against current code.
- Sweeping every consumer of a changed contract across every test
framework this repo uses, and the local-test ceiling around E2E/
live-instance/timing-dependent regressions that only ever surface
in CI.
- Verifying every CI/review finding against current code before
acting on it (stale findings already fixed by a later commit, and
wrong findings that would reopen the very vulnerability being
fixed), rather than complying by default.
- Confirming CI is green on the actual merge commit post-merge, and
leaving advisory publication timing to the maintainer.
Carries forward the same disclosure-caution rule as triage (no GHSA/
CVE ID or exploit specifics in branch names, commits, or PR text).
Sanity-checked against two fictional (not real) scenarios in isolated
worktrees before landing: a path-traversal report and a broken-access-
control report, one of which correctly surfaced as a false positive
against current code partway through, exercising the skill's guidance
for that case directly.
* Address Codex review comments
- Generalize the AskUserQuestion reference: name it as the Claude Code
tool it actually is, rather than implying it's universally available
across any agent that might use this skill (P2 finding: Codex itself
has no such tool and would either stall or silently pick an option
on this instruction as originally worded).
- Make explicit that opening a public PR before the advisory is
published is itself a disclosure of the code change, regardless of
how carefully the branch/PR are named, since the diff is visible to
anyone with read access. Document this project's accepted tradeoff
(ship the fix, hold the advisory's publication separately) as a
deliberate choice rather than an oversight, while flagging that it's
specific to this project's normal case and worth confirming with the
user when an advisory's severity or threat model argues for more
caution (P1 finding).
* Address second round of Codex review comments
- Stage 2's local-test step now explicitly instructs running the E2E
suite (composer test -- --testsuite E2E, npm run pw:run) before
pushing when the environment actually has the needed credentials/live
instance, rather than only noting the limitation without ever
telling the agent to use it when available (P2 finding).
- Added an explicit gate before Stage 4: CI-green and resolved review
threads aren't the same as mergeable. CONTRIBUTING.md requires two
maintainer reviews before merge, a separate approval gate this skill
shouldn't merge around even where the tooling has permission to (P2
finding).
* Run frontend checks too when a fix touches browser/theme code
Stage 2's local-test step was PHP-only (composer test/phpstan/CS-Fixer).
Add the frontend equivalent (npm run test:js, npm run build) for when
the fix or its regression test touches browser/theme code instead, so
a broken frontend change doesn't ship alongside a passing PHP suite
(Codex finding).
* Add security-advisory-triage skill
Codifies the two-stage workflow for handling a reported GitHub Security
Advisory: verify it against current code with file:line evidence, then
refine its title/description/CVSS/CWE metadata to match this repo's
actual publish conventions before applying it. Includes a living
house-style reference derived from this repo's own published advisories.
Lives at .agents/skills/ with .claude/skills symlinked to it, matching
the existing AGENTS.md/.claude/CLAUDE.md convention for cross-tool
agent support.
* Fix spellcheck: add GHSA/macrovector/rubygems, ignore skills reference dir
The skills reference file legitimately contains GHSA advisory IDs as
provenance examples; those are opaque identifiers, not real words, and
new ones will keep appearing as more advisories get referenced there
over time, so excluding the directory from spellcheck avoids repeated
dictionary churn rather than fixing it once.
* Address CodeRabbit review on security-advisory-triage skill
- Stage 1's current-code check now requires an exact revision match
(fetch + clean worktree + exact rev-parse comparison) instead of only
an ancestor check, which could pass against a stale or divergent base.
- Stage 1's related-history step now queries each advisory state
explicitly with --method GET, and calls out that gh api silently
sends a POST (hitting the advisory-creation endpoint) when -f is used
without --method GET. Verified directly against this repo: an
unfiltered call does return every state for a maintainer-authenticated
token, but explicit per-state queries are more portable.
- Confidentiality note broadened to cover chat/review/tool output, not
just committed artifacts.
- CVSS guidance now distinguishes the nested read path
(cvss_severities.cvss_v3/cvss_v4) from the flat write field
(cvss_vector_string), and notes severity/cvss_vector_string are
mutually exclusive on write.
- The live-advisory PATCH checklist now lists the actual top-level
fields (summary, description, cwe_ids, cvss_vector_string/severity)
instead of vaguely saying 'the description body', notes
vulnerabilities is an array rather than assuming index [0] is
everything, and says to PATCH the complete object per entry rather
than a partial one.
- Added language tags to two fenced code blocks (shell, markdown) per
markdownlint MD040.
* Fix data-loss gap in vulnerabilities PATCH guidance
The 'send the complete object' instruction added last round listed only
four fields (package.ecosystem/name, vulnerable_version_range,
patched_versions) and omitted vulnerable_functions, which is a real
field on the object (confirmed: this session's own successful PATCH
sent it as an empty array). Following that guidance literally on an
advisory where vulnerable_functions is actually populated would erase
it. Added the missing field.
Adam Daley