Skip to content

feat: add Cursor dual-target marketplace manifests - #1835

Merged
kyle-sexton merged 7 commits into
mainfrom
feat/cursor-dual-manifests
Jul 30, 2026
Merged

feat: add Cursor dual-target marketplace manifests#1835
kyle-sexton merged 7 commits into
mainfrom
feat/cursor-dual-manifests

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

No linked issue

Summary

Add generated .cursor-plugin/ marketplace and per-plugin manifests so this GitHub repo can be imported as a Cursor personal or Team Marketplace, while Claude Code continues to own .claude-plugin/ as SSOT.

Fix

  • Add scripts/generate-cursor-manifests.mjs (write + --check) that copies name/version/description/author/license/keywords and marketplace category/tags/displayName from Claude manifests, stripping Claude-only fields (
    elevance, defaultEnabled, userConfig, $schema).
  • Commit generated .cursor-plugin/marketplace.json and 61 plugins/*/.cursor-plugin/plugin.json files.
  • Wire --check into scripts/validate-plugins.sh; include Cursor manifests in the duplicate-key gate.
  • Document Cursor install/refresh (and stale-pin workaround) in README; note regeneration in CLAUDE.md, migration playbook, catalog taxonomy, and OFFICIAL-DOCS.

Verification

ode scripts/generate-cursor-manifests.mjs --check passes

  • Metadata audit: 61/61 names kebab-case, dir=name, Cursor descriptions match Claude plugin.json, sparse displayNames preserved
  • python scripts/check-manifest-duplicate-keys.py reports 124 manifests, no duplicate keys
  • Relative sources, unique names, and per-plugin Cursor manifests present

Related

N/A

Generate .cursor-plugin catalogs from Claude SSOTs so the deployed GitHub marketplace can be imported in Cursor Customize / Team Marketplaces.

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 12s —— View job


Security Review

  • Gather context (PR diff, changed files)
  • Review scripts/generate-cursor-manifests.mjs (new script, file I/O, JSON parsing)
  • Review scripts/validate-plugins.sh changes
  • Review scripts/check-manifest-duplicate-keys.py changes
  • Review generated .cursor-plugin/*.json manifests for injected/unexpected content
  • Review doc changes for accuracy/security-relevant claims
  • Post final security review findings

No security issues found.

Reviewed against the trust-boundary / logic / data-flow lens (unpinned actions, workflow triggers, and permissions are out of scope here per zizmor's advisory lane — and this PR touches no .github/workflows/* files anyway):

  • scripts/generate-cursor-manifests.mjs only reads/writes local repo files (node:fs), never shells out, never evals, and never touches the network. It parses .claude-plugin/marketplace.json and per-plugin plugin.json — both already-trusted, maintainer-authored SSOT files, not runtime/external input — and writes derived .cursor-plugin/ manifests. Copied fields (PLUGIN_FIELDS/ENTRY_FIELDS) are a fixed allowlist, so no unexpected data (secrets, executable content) can flow through; Claude-only fields (relevance, defaultEnabled, userConfig, $schema) are correctly stripped, not leaked.
  • The path construction in pluginManifestPath()/listCursorPluginFiles()/writeArtifacts() derives paths from entry.source in the trusted marketplace.json and from readdirSync results under plugins/ — no traversal reachable from untrusted input, and the orphan-cleanup rmSync calls are scoped to paths this same script just enumerated under plugins/*/.cursor-plugin/.
  • --check mode (wired into scripts/validate-plugins.sh:19) is read-only — CI never runs the write path, so even a malicious PR editing marketplace.json can't get this script to write outside the repo during CI.
  • scripts/check-manifest-duplicate-keys.py changes only add two new glob patterns to an existing allowlist-style scan — no new parsing or execution behavior.
  • The 61 generated .cursor-plugin/plugin.json files and .cursor-plugin/marketplace.json contain only plain metadata (name/version/description/author/license/keywords/category/tags) copied from already-committed Claude manifests — spot-checked and grepped across the diff for credential-like strings, no matches (the one "secrets" hit is just a plugin's own descriptive text, e.g. plugins/guardrails).
  • Doc-only changes (README.md, CLAUDE.md, docs/CATALOG-TAXONOMY.md, docs/MIGRATION-PLAYBOOK.md, docs/OFFICIAL-DOCS.md) are prose describing the new generation contract and Cursor install flow; nothing there executes or is security-relevant beyond documentation accuracy (outside this review's scope).

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 50s —— View job


Code Review

  • Gather context (PR diff, changed files)
  • Review scripts/generate-cursor-manifests.mjs for correctness/error handling
  • Review scripts/validate-plugins.sh and scripts/check-manifest-duplicate-keys.py changes
  • Spot-check generated .cursor-plugin/*.json manifests for correctness
  • Review doc changes for accuracy
  • Post final review findings

Reviewed for correctness, architecture, error handling, test coverage, and maintainability (security is covered separately by the security-review lane). Scope limited to the changed files.

Summary

The generator (scripts/generate-cursor-manifests.mjs) is solid: the field allowlists (PLUGIN_FIELDS/ENTRY_FIELDS) correctly strip Claude-only fields, --check mode diffs byte-for-byte against stableStringify output, orphan cleanup is scoped to paths the script itself enumerated, and the required-field guards (name, description, catalog-name match) fail loudly instead of silently emitting a broken manifest. I spot-checked several generated outputs against their Claude sources (markdown-format, discipline, planning, and the five sparse-displayName plugins called out in the PR body — eol-normalizer, mcp-tools, tdd, kindle-dedrm, ai-briefing) and all matched expectations; counted 61/61 .claude-plugin/.cursor-plugin plugin.json pairs. Docs (README.md, CLAUDE.md, docs/CATALOG-TAXONOMY.md, docs/MIGRATION-PLAYBOOK.md, docs/OFFICIAL-DOCS.md) are accurate and consistent with the fresh-docs mandate (cite Cursor's docs URLs with a verified date).

One test-coverage gap worth addressing before merge:

1. New DEFAULT_GLOBS entries in check-manifest-duplicate-keys.py aren't exercised by the test suite
scripts/check-manifest-duplicate-keys.py#L60-L65 adds plugins/*/.cursor-plugin/plugin.json and .cursor-plugin/marketplace.json to DEFAULT_GLOBS. scripts/test_check_manifest_duplicate_keys.py has a dedicated test for default-glob discovery, test_default_discovery_covers_plugin_and_marketplace_manifests, but it only writes .claude-plugin fixtures — it wasn't extended to cover the two new glob patterns. Because the existing fixtures never create a .cursor-plugin file, the suite would pass unchanged even if a Cursor glob were typo'd or pointed at the wrong path segment, silently defeating the duplicate-key gate for Cursor manifests. Worth adding a fixture + assertion (duplicate key inside a .cursor-plugin/plugin.json, or an extension of the existing "all clean" count check) so a future glob regression is actually caught.

Minor, non-blocking observations:

  • scripts/generate-cursor-manifests.mjs has meaningful branching logic (write vs. --check, drift diffing, orphan removal) but ships with no paired *.test.sh, unlike its closest sibling scripts/generate-cheatsheet.mjs, which has generate-cheatsheet.test.sh wired into CI (.github/workflows/ci.yml:579). Not a hard blocker — generate-catalog.mjs is similarly untested today — but this is the newest and most logic-heavy of the three generators, so it's the best candidate to start closing that gap.
  • README's Cursor troubleshooting step (README.md) gives Unix-style cache paths (~/.cursor/plugins/...) for the "stale pin" workaround with no Windows equivalent noted. Given this repo also documents Windows-specific tooling elsewhere (e.g. machine-health), worth a follow-up if Cursor's Windows cache path differs — not blocking since it's a documented workaround, not a schema contract.

No correctness bugs found in the generation logic itself.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9a1c8136c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/generate-cursor-manifests.mjs Outdated
Comment thread scripts/generate-cursor-manifests.mjs Outdated
Satisfy the shebang exec-bit hygiene gate (100755).

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 35s —— View job


Security Review

  • Gather context (PR diff, changed files)
  • Review scripts/generate-cursor-manifests.mjs (file I/O, JSON parsing, path handling)
  • Review scripts/validate-plugins.sh and scripts/check-manifest-duplicate-keys.py changes
  • Review generated .cursor-plugin/*.json manifests for injected/unexpected content
  • Review doc changes for security-relevant claims
  • Post final security review findings

No security issues found.

This is a re-review of the same commit set already reviewed at b9a1c81; the only new commit (f9f7f7f) is a file-mode change (100644100755 on generate-cursor-manifests.mjs, no content diff), so the prior findings still hold:

  • scripts/generate-cursor-manifests.mjs only does local node:fs reads/writes, no shell-out, no eval, no network. It reads already-trusted, maintainer-authored .claude-plugin/marketplace.json / plugin.json files and writes derived .cursor-plugin/ manifests through a fixed field allowlist (PLUGIN_FIELDS/ENTRY_FIELDS), so Claude-only fields (relevance, defaultEnabled, userConfig, $schema) are stripped rather than leaked, and no unexpected data can flow through.
    • The one theoretical path-traversal note: pluginManifestPath() (line 71-74) joins root with entry.source from marketplace.json without validating it stays under plugins/. In CI this is inert — validate-plugins.sh only ever invokes --check (read-only compare, scripts/validate-plugins.sh#L19), never the write path. It would only matter if a maintainer manually ran the write mode against an untrusted marketplace.json (e.g., checked out from an unreviewed fork PR) — low likelihood, not exploitable through CI or the published plugin surface. Not flagging as a finding, noting for awareness only.
  • scripts/check-manifest-duplicate-keys.py changes only extend an existing glob allowlist with two new patterns (plugins/*/.cursor-plugin/plugin.json, .cursor-plugin/marketplace.json) — no new parsing/execution surface.
  • scripts/validate-plugins.sh change is a single additional read-only --check invocation ahead of the existing gates — no eval, no untrusted input into the shell.
  • Spot-checked the generated .cursor-plugin/marketplace.json and dometrain/miro manifests (the two Codex flagged for functional correctness — unresolved ${user_config.*} / ${CLAUDE_PLUGIN_ROOT} placeholders in .mcp.json once userConfig is stripped): this is a broken-credential/won't-start problem, not a security one — it degrades to sending/using a literal placeholder string, not a real secret, and doesn't leak anything. Grepped all generated manifests and the generator script for secret-shaped strings (password|secret|token|api[_-]?key|bearer|credential) — only benign keyword/description hits (e.g. plugins/guardrails), no embedded credentials. All source/homepage/repository fields are relative repo paths or already-public URLs copied verbatim from the trusted Claude SSOT.
  • Doc-only changes (README.md, CLAUDE.md, docs/CATALOG-TAXONOMY.md, docs/MIGRATION-PLAYBOOK.md, docs/OFFICIAL-DOCS.md) are prose describing the generation contract and Cursor install flow, including an explicit caveat that hooks/userConfig aren't portable — nothing security-relevant beyond documentation accuracy (out of scope for this review).
  • No .github/workflows/* files are touched by this PR.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 59s —— View job


Code Review

  • Gather context (diff since last code review, current HEAD state)
  • Check status of previously-flagged test-coverage gap (check-manifest-duplicate-keys.py DEFAULT_GLOBS)
  • Assess Codex P1 correctness findings (hook portability, MCP userConfig credentials) against CLAUDE.md scope
  • Post final review findings

The only commit since the last full code review (b9a1c81) is f9f7f7f, a file-mode change (generate-cursor-manifests.mjs → executable) with no content diff, so the generator logic, validate-plugins.sh/check-manifest-duplicate-keys.py wiring, and generated manifests are unchanged from the prior review. Re-verified against current HEAD (f9f7f7f); two items from that review remain open, plus the two Codex P1s hold up on independent check:

1. Generated descriptions still advertise Claude-only behavior to Cursor users (echoing Codex, IMPORTANT)
scripts/generate-cursor-manifests.mjs#L76-L83 copies description verbatim with no portability filtering, and #L123-L124 reuses it for the marketplace entry. Checked the actual generated output:

  • plugins/markdown-format/.cursor-plugin/plugin.json#L4 promises "Auto-format and lint Markdown on edit," but that behavior is implemented entirely by plugins/markdown-format/hooks/hooks.json, which isn't portable.
  • plugins/dometrain/.cursor-plugin/plugin.json#L4 is worse — it names a Claude-specific mechanism by name ("Claude Code's native masked userConfig prompt") in a manifest whose own userConfig block was just stripped by this same generator, so the described credential flow cannot exist in the artifact it's shipped in.
  • README.md#L33 documents the general caveat ("Claude-specific hooks and userConfig fields are not portable"), but that's a blanket footnote a Cursor user won't see while browsing a specific plugin's marketplace description — it doesn't prevent per-plugin copy from actively promising unavailable behavior.
    This is a correctness/maintainability gap in the generator, not just a docs nit: either exclude hook/MCP-credential-dependent plugins from the Cursor catalog, or add a description-rewrite step (e.g., strip/replace hook- and userConfig-specific clauses) so generated copy matches what actually runs under Cursor.

2. dometrain/miro ship non-functional MCP configs under Cursor (echoing Codex, IMPORTANT)
Same root cause, functional angle: plugins/dometrain/.mcp.json and plugins/miro/.mcp.json still reference ${user_config.dometrain_api_key} / ${user_config.miro_api_token} (and Miro's command also uses ${CLAUDE_PLUGIN_ROOT}), but Cursor has no userConfig substitution mechanism and the generator strips userConfig from the Cursor manifest entirely. A Cursor user who installs either plugin gets an MCP server that can't authenticate or resolve its own command path — the manifest markets a working integration that isn't reachable through Cursor's install path. Same fix options as above (exclude these plugins from Cursor export until there's a portable config story, or generate Cursor-specific substitutions).

3. Test-coverage gap for the new DEFAULT_GLOBS entries is still unaddressed (unchanged from prior review)
scripts/check-manifest-duplicate-keys.py#L60-L65 adds plugins/*/.cursor-plugin/plugin.json and .cursor-plugin/marketplace.json to the default glob set. scripts/test_check_manifest_duplicate_keys.py#L173-L193 (test_default_discovery_covers_plugin_and_marketplace_manifests) still only writes .claude-plugin fixtures, so a typo'd or misdirected Cursor glob would pass this suite unchanged. Not re-flagging as new, just confirming it hasn't been picked up yet.

Minor, non-blocking (unchanged from prior review):

  • generate-cursor-manifests.mjs still has no paired *.test.sh, unlike generate-cheatsheet.mjs.
  • README's Cursor stale-pin workaround (README.md#L28-L31) is Unix-only cache paths with no Windows equivalent.

No new correctness bugs introduced by the f9f7f7f chmod commit itself.

Fix this →

… shims

Split the generator into scripts/cursor-export/, port MCP to Cursor variables/mcp.json, and point Claude hook plugins at empty Cursor-native stubs so Cursor never parses Claude hooks/hooks.json.

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

The check is red on purpose. It certifies that a security pass ran, and this one did not complete, so it cannot report success. Where this check is required, merging is blocked until a review actually finishes.

Re-running the job, or pushing a new commit, will retry the review. An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

  • rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
  • a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-run the job to retry the review. A new push does not re-trigger this lane.
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).

kyle-sexton and others added 2 commits July 30, 2026 17:53
Drop the test.mjs shebang (run via node), use ASCII Plugins -> Configure in generated copy, and enrich --check drift details for CI.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

kyle-sexton and others added 2 commits July 30, 2026 18:09
CI merges main into the PR; Cursor plugin.json versions must track the Claude SSOTs or --check fails.

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@kyle-sexton
kyle-sexton merged commit b6c4b58 into main Jul 30, 2026
30 of 31 checks passed
@kyle-sexton
kyle-sexton deleted the feat/cursor-dual-manifests branch July 30, 2026 22:17
kyle-sexton added a commit that referenced this pull request Jul 30, 2026
## Summary
- Revert squash merge `b6c4b58` (#1835) so `claude-code-plugins` stays
Claude Code–only.
- Removes generated `.cursor-plugin/**`, `scripts/cursor-export/`,
`scripts/generate-cursor-manifests.mjs`, Cursor-ported `mcp.json` files,
and related docs/gate wiring.
- Claude SSOT (`.claude-plugin/**`, plugin hooks/MCP) is unchanged.

## Why
Cursor dual-target in this marketplace caused overlapping install paths
and mixed contracts. Cursor-native marketplace work will live in a
separate governed repo (via `github-iac`), with its own migrate/adapt
playbook inspired by this catalog.

## Test plan
- [ ] CI green on this revert branch
- [ ] Confirm `.claude-plugin/marketplace.json` and plugin Claude
manifests still present
- [ ] Confirm no `.cursor-plugin/` paths remain on the tree
- [ ] Spot-check README/CLAUDE.md no longer document Cursor marketplace
install for this repo

## Related
- Reverts #1835
- Companion: melodic-software/github-iac#251 (`cursor-plugins`
governance)

No linked issue
kyle-sexton added a commit that referenced this pull request Aug 17, 2026
…ributions (#2843)

Closes #2837. Closes #2833.

## Summary

Three defects in the silent-revert canary, plus one unfiled
harness-safety fix
in a file this change already owns. Branched off `2de57a379` (#2832),
which had
already merged.

## Fix

## 1. `declares_removal()` could not read the only revert subject that
merges here (#2837)

The detector accepted three intent forms. This repo is squash-only with
`squash_merge_commit_title: PR_TITLE`, so the squash subject is the PR
title,
and `.github/workflows/pr-title.yml` gates every title through a
required
Conventional-Commits check whose default type list is all-lowercase — it
admits
`revert:` and contains nothing a `Revert "…"` subject could match. So a
deliberate revert reached `main` wearing a subject the detector could
not read.

`declares_removal()` now also accepts the Conventional-Commits revert
type,
anchored at the start of the **subject**, requiring the literal
lowercase token,
its optional `(scope)` and/or `!`, its colon, and a non-empty
description:

```
^revert(\([^()]+\))?!?:[[:space:]]*[^[:space:]]
```

Never a substring search for "revert" — kept exactly as constrained as
the three
forms beside it.

### Before / after on the repo's one real deliberate revert

`1d1fca6e8` — `revert: remove Cursor dual-target marketplace manifests
(#1835) (#1839)`

Before (at `2de57a379`, shipped detector, thresholds unmodified):

```
$ bash scripts/check-silent-revert.sh --commit 1d1fca6

SILENT REVERT SUSPECTED

  removed by   1d1fca6  revert: remove Cursor dual-target marketplace manifests (#1835) (#1839)
               2026-07-30 19:41:47 -0400
  content from b6c4b58  feat: add Cursor dual-target marketplace manifests (#1835)
               2026-07-30 18:17:36 -0400  (1 commit(s) earlier on main)
  lines lost   3361  (threshold 200, window 40 commits)
  [... 102 files, sample block and "What to do" block elided; 130 lines total ...]

EXIT=1
```

After:

```
$ bash scripts/check-silent-revert.sh --commit 1d1fca6
declared 1d1fca6 revert: remove Cursor dual-target marketplace manifests (#1835) (#1839)
         removal is declared: the subject carries the Conventional-Commits revert type
EXIT=0
```

### Suppression is not widened over anything the corpus records

- No recorded incident has a `revert`-prefixed subject — `f603880da`
`fix(disk-hygiene): …`, `9239f1541` `feat(disk-hygiene): …`, `cc58cbc53`
`fix(repo-fleet-hygiene): …`, `c8470efd0` `docs(conventions): …`. All
four rows
  still hold (green run below).
- Neither acknowledgment-file commit is revert-prefixed either —
`6f0a31109`
`fix(repo-fleet-hygiene): …`, `91e77fc16` `fix(hook-utils): …` — so no
ack row
goes dead now that `declares_removal()` short-circuits ahead of
`ack_reason()`.
- The header's "fires on 5 commits — 1%" calibration figure is therefore
  unchanged; none of those five is revert-prefixed.

New tests pin both directions. `revert:`, `revert(scope):`, `revert!:`
and
`revert(scope)!:` suppress; `feat: do not revert the alpha guard (#99)`,
`reverted: drop the alpha guard (#99)`, `Revert: drop the alpha guard
(#99)` and
a bare `revert:` with no description all still fire. The pre-existing
case only
covered a *body* mention of "revert"; the subject is what the new form
reads, so
that is where a substring bug would widen.

## 2. `verify_known_incidents` asserted only "something fired" (#2833)

A `fires` row passed on `scan_commit`'s exit status while the note
beside it
named a specific culprit and a specific line count that nothing checked.
On
`cc58cbc53`, whose deletions trace to two culprits, losing the
`eda5ae5ed`
attribution entirely would still have printed `ok` on the surviving
`bfb66beb8`
finding — the canary announcing a reproduction it did not perform.

A `fires` row may now carry a bracketed attribution expectation after
its sha:

```
fires <sha> [<culprit-full-sha>=<blamed-lines>,<culprit-full-sha>=<blamed-lines>] <note>
```

and the replay asserts the run's findings are **exactly** that set —
same
culprits, same per-culprit counts, no extras, no omissions. Full
40-character
culprit shas only, the same discipline `silent-revert-acknowledged.txt`
uses.

Counts come from a new `FINDINGS_SINK` file that `report_finding`
appends
`<full-culprit-sha> <count>` to, not from scraping the human report —
the report
prints a 9-character abbreviation, which is not enough sha to assert on.
Nothing
else sets `FINDINGS_SINK`, so `--commit` and range mode are
byte-identical.

Every recorded figure was **measured, not transcribed from the notes** —
the
sink was wired first and the observed values recorded:

```
f603880 -> a95f240 346
9239f15 -> f603880 451
cc58cbc -> bfb66be 853
             eda5ae5 298
```

Three of the four agree with the notes #2832 corrected; the fourth is
298 rather
than 301, for the reason in section 5. A malformed field is exit 2
(cannot
run), never a FAIL and never a pass — a silently misread expectation is
the same
false green this file exists to remove. The field is optional so a row
can be
pinned before its attribution is measured, but a new well-formedness
assertion
requires every *shipped* `fires` row to carry one.

## 3. The workflow header stated a reason that was not true (unfiled)

`.github/workflows/silent-revert-canary.yml` asserted *"There is no
`pull_request` trigger, so it can never gate a PR"* — while its own
`on:` block
has a paths-filtered `pull_request` trigger, added by #2808 to run the
detector's
unit tests, and explained at length 20 lines further down in the same
file. The
conclusion holds for a different reason: both scan steps are gated
`if: github.event_name != 'pull_request'`, and the lane sits outside
`ci.yml`
and its `ci-status` aggregate. Wording corrected to the actual reason.

**No trigger and no `if:` changed.** `git diff` on that file is comment
lines
only — one hunk, `@@ -13,6 +13,9 @@`, entirely inside the `#` header.

## 4. The detector's line counts depended on ambient git config
(unfiled, found by #2833's new assertion)

The first CI run of this branch went red, and the failure is the most
valuable
thing in this PR. The runner reported the `eda5ae5ed` attribution as
**298**
lines where my machine measured **301**:

```
FAIL cc58cbc fires, but NOT as recorded
     recorded attribution:   eda5ae5…  301
     what the detector reported:  eda5ae5…  298
```

Cause: `attribute_file` called bare `git diff --unified=0`, so it
inherited
whatever `diff.algorithm` the caller's config carried. I have
`diff.algorithm = histogram` set globally; CI has nothing set and
therefore uses
git's default `myers`. The algorithm changes which lines a hunk calls
deleted,
so it changes the per-culprit counts this canary **thresholds on**.
Reproduced
directly:

```
$ GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.algorithm GIT_CONFIG_VALUE_0=myers \
    FINDINGS_SINK=… scripts/check-silent-revert.sh --commit cc58cbcbfb66be…  853
eda5ae5…  298      # 301 under histogram
```

Three lines is harmless in itself. The principle is not: the same drift
can
carry a count across the 200-line threshold, so a commit could fire on
one
machine and stay silent on another, and the header's "fires on 5 commits
over
500 — 1%" calibration only ever described one algorithm.

`attribute_file` now pins `--diff-algorithm=myers -M` explicitly, and
the file
enumeration pins `-M` too. **Both are git's defaults, so this does not
change
what CI detects today** — CI already had no `diff.algorithm` set. It
makes a
local run match CI, not the reverse. `-M` covers the same exposure for
rename
detection, which the existing header calls "load-bearing rather than
incidental": `diff.renames = false` in a developer's config would
decompose a
`git mv` into delete + add and make relocating a large recent file fire.

The recorded figure is now **298**, and the prose figures in the script
header
and the corpus are corrected with the refutation attached, so nobody
re-measuring on a histogram machine "corrects" it back to 301.

Worth stating plainly: nothing asked for this. #2833's exact-count
assertion
turned a silent, config-dependent divergence into a red build on its
first run —
which is precisely the argument for asserting attributions instead of
exit
status.

## 5. The test harness could commit the developer's work as `test
<t@t.test>` (unfiled)

Found the hard way while developing this. `mk_repo` is called as
`repo="$(mk_repo)"`, so a `return 1` inside the command substitution
cannot abort
the suite — the caller just gets `""`. And `""` is not inert: `git -C
""` is
documented as a no-op, so the next `add -A` + `commit` staged and
committed my
uncommitted work into the checkout, authored `test <t@t.test>`. Such a
commit
cannot be pushed here — it fails `required_signatures` with `no_user`.

`mk_repo` now yields a path derived from `SELF_DIR` that does not exist,
so every
git call against it fails loudly and the assertions go red —
fail-closed, which
is what a harness that cannot build its fixture should do. Scoped to
this file
only; if the same `repo="$(mk_repo)"` shape exists in sibling harnesses
that is a
separate follow-up.

## Verification

`scripts/check-silent-revert.sh --verify-known-incidents`, run on a
machine with
`diff.algorithm = histogram` set globally — it now agrees with CI
exactly,
because the flags are pinned:

```
ok   f603880 fires as recorded, 1 attribution(s) reproduced exactly  (#2639 dropped #2635 (346 blamed lines), 13 minutes later)
ok   9239f15 fires as recorded, 1 attribution(s) reproduced exactly  (#2641 dropped #2639 (451 blamed lines), 10 minutes later)
ok   cc58cbc fires as recorded, 2 attribution(s) reproduced exactly  (#2633 dropped #2644's rollups (853 blamed lines) and #2642's GraphQL merge evidence (298); already recorded in #2656)
ok   c8470ef stays clean as recorded  (docs(conventions) rewrote 129 lines of a doc #2679 had just added)

Canary reproduces every recorded incident at the shipped settings.
```

Injected failure against the **shipped** corpus (`853` changed to `852`
in a
copy) — the mechanism is proven on real rows, not only on synthetic
fixtures:

```
$ SILENT_REVERT_INCIDENTS=<copy with 298 -> 297> scripts/check-silent-revert.sh --verify-known-incidents
ok   f603880 fires as recorded, 1 attribution(s) reproduced exactly  (...)
ok   9239f15 fires as recorded, 1 attribution(s) reproduced exactly  (...)
FAIL cc58cbc fires, but NOT as recorded  (#2633 dropped #2644's rollups (853 blamed lines) and #2642's GraphQL merge evidence (298); already recorded in #2656)
     recorded attribution:
       bfb66be 853
       eda5ae5 297
     what the detector reported:
       bfb66be 853
       eda5ae5 298
     A row that fires for the wrong reason is not a reproduction.
     Do NOT edit the row to match; find out why the attribution moved.
ok   c8470ef stays clean as recorded  (...)

The canary no longer reproduces the incidents it was built for.
Do not relax the recorded expectations to make this pass.
EXIT=1
```

The commit still fires — exit status alone would have passed this row.
Note the
row is red on a **one-line** discrepancy in one of two attributions,
which is
exactly the regression #2833 describes.

`scripts/check-silent-revert.test.sh`: **49 passed, 0 failed**,
including
`replay fails when the finding is attributed to a different culprit`,
`replay fails when the recorded line count no longer reproduces`, and
`replay fails when one of two recorded attributions stops reproducing`.
So the
assertion is proven by permanent tests, not only by a one-off injection.

## 6. Review follow-up: an unterminated attribution field read as
*absent*

Both automated review lanes independently flagged the same real gap, and
they
were right. A `fires` row whose field opened with `[` but never closed
it failed
the `[[ "$rest" == \[*\]* ]]` glob, so `attribution` stayed empty, the
remainder
became free-text `note`, and the row fell back to passing on exit status
alone —
reintroducing the exact pre-#2833 gap by the one route nobody would look
at, and
contradicting the contract documented directly above it.

A leading `[` now COMMITS the row to carrying an attribution;
unterminated takes
the malformed path. Reproduced against the real corpus with the closing
bracket
stripped from the `f603880da` row:

```
check-silent-revert: unterminated attribution field for f603880… (no closing ']'): [a95f240…=346 #2639 dropped #2635 …
EXIT=2
```

The four malformed shapes already pinned all carried a closing `]`, so
this one
was untested; `[<sha>=40` and a bare `[` are now pinned too. The shipped
corpus
was never at risk — `t_shipped_data_files_are_wellformed`'s regex covers
it —
but that is a separate layer and does not hold for a custom
`SILENT_REVERT_INCIDENTS`.

## 7. Verification follow-up: `git blame` was still ambient-config
dependent

Fresh-context verification of section 4 found that fix was only half of
one.
Pinning the diff flags left `attribute_file`'s `git blame` call bare, so
`blame.ignoreRevsFile` — an ordinary setting in any repo carrying a
bulk-reformat commit — still decided the per-culprit counts the replay
now
asserts on. On the real corpus, with that setting naming `bfb66beb8`:

| culprit | pinned | with `blame.ignoreRevsFile` |
| --- | ---: | ---: |
| `bfb66beb8…` | 853 | **259** |
| `eda5ae5ed…` | 298 | **322** |

On a synthetic fixture it is worse than a wrong number. With the pin
absent and
that config present, the detector reports **no finding at all** on a
genuine
silent revert:

| | clean config | hostile config |
| --- | --- | --- |
| pinned | `culprit 40` | `culprit 40` |
| unpinned | `culprit 40` | **(nothing — the canary goes silent)** |

That is a false green reached through the developer's own gitconfig —
the
precise failure this canary exists to remove.

**The obvious fix does not work, and the comment says so.**
`-c blame.ignoreRevsFile=` does *not* clear it: the documented "an empty
file
name resets the list" applies to the **option**, and the `-c` form was
measured
leaving the hostile value fully in effect (853 → 259 with the reset
supposedly
applied). Only `--no-ignore-revs-file` actually resets. The symmetry
with the
`-c` pins above is wrong here and is deliberately not used.

`t_counts_are_immune_to_ambient_git_config` pins the property: the same
fixture
scanned twice, once under a hostile `GIT_CONFIG_GLOBAL` setting
`blame.ignoreRevsFile`, `diff.algorithm` and `diff.renames`, asserting
identical
exit status and identical per-culprit findings. **Confirmed
discriminating** —
with the blame pin stripped it fails, and it fails because the hostile
run
reports nothing at all.

`shellcheck`, `actionlint`, `typos`, `bash -n` and
`scripts/check-shell-portability.sh` all pass.

## Test plan

Run from a clean checkout of this branch, with `unset GIT_DIR
GIT_WORK_TREE`:

1. `bash scripts/check-silent-revert.test.sh` — the detector's own unit
suite.
Expect **49 passed, 0 failed**. Covers all four `revert:` spellings, the
   four negative subject cases (`feat: do not revert ...`, `reverted:`,
`Revert:`, bare `revert:`), the wrong-culprit / wrong-count / missing-
   attribution replay regressions, all six malformed-field shapes, and
   `t_counts_are_immune_to_ambient_git_config`.
2. `bash scripts/check-silent-revert.sh --verify-known-incidents` — the
real
   corpus. Expect exit 0 with all three `fires` rows reporting
`attribution(s) reproduced exactly` and the `clean` row staying clean.
3. Negative control for step 2: edit
`scripts/silent-revert-incidents.txt` to
inject a wrong culprit sha (leaving the count correct, so the commit
still
   fires) and, separately, a wrong line count. Each must exit **1** with
   `fires, but NOT as recorded`. Restore the file afterwards.
4. Set `diff.algorithm = histogram` in global git config and repeat
steps 1-2.
   The numbers must not move — that is what the new flag pins buy.
5. `git diff origin/main...HEAD --
.github/workflows/silent-revert-canary.yml`
   must show comment-only changes; no executable YAML line may differ.

## Related

- Refs #2808 — the PR that merged the canary and its three intent forms.
- Refs #2832 / #2831 — the corpus-attribution correction this branches
off;
  #2833 was raised in its review.
- Refs #2691 — the original silent-revert audit the corpus is built
from.
- Refs #1839 — the deliberate revert (`1d1fca6e8`) used as the
real-history
  fixture for #2837.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant