Skip to content

fix(skills): preserve Core contracts across catalog refreshes - #1905

Open
cobmojo wants to merge 7 commits into
developfrom
cursor/skills-catalog-signed-61e9
Open

cobmojo wants to merge 7 commits into
developfrom
cursor/skills-catalog-signed-61e9

Conversation

@cobmojo

@cobmojo cobmojo commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Catalog refreshes could overwrite Core's discovery and UI/payment guidance, leave partial files after a failure, or hide a failed rollback behind temporary-directory cleanup. This change preserves Core adapters through refresh and sync, makes directory/companion/lock updates recoverable, and reports failed recovery accurately.

The catalog keeps its 128 upstream records and the separately merged architecture record. Canonical sources and all three runtime mirrors are reconciled, including explicit-only routing, concrete-task handling, Base UI composition, the existing Sonner/shared utility owners and Stripe's actual API owner. The menu example adapter validates every replacement before writing. Binary examples retain their verified original bytes.

Scanner annotations no longer become rendered JSX text or alter continuation, heredoc, template and YAML payloads. Uncertain code regions remain byte-exact and scanner-visible instead of guessing a terminator. Ecosystem publication failures now propagate as failures, and entry-aware checks preserve unexpected live or dangling symlinks and complete recovery copies.

This carrier preserves the reviewed unique work from #1430, #1862, #1863, #1902 and #1904. That includes the narrow Eve relation-UUID validation repair: schema-validated identifiers are excluded from text scanning while sensitive content and invalid or inaccessible references still reject. #1915 carries the identical validator repair for its shared build qualification; the remaining design packs and tooling still belong to this integration. Predecessor PRs will be closed only after actual merged preservation is verified.

Validation on published head 03c343d7e9af122fd21c0bc685222107cb57b348, based on bd9acc44313761d3371996c85376373782da02fb:

  • Normal full pre-push bun run ci:preflight passed: all three application builds, 4,473 tests, and four existing skips. Formatting, lint, types, OpenSpec and generated-mirror checks passed without bypasses.
  • Actual CLI fault-injection tests cover partial backup, cross-device moves, failed publication/rollback/cleanup, occupied mirror destinations and interrupted removal. Raw fixture, idempotence and drift controls cover the retained adapters.
  • Independent comparisons verified the complete predecessor preservation map and the merged architecture guidance, including modes, deletions, provenance and generated output. Source mutations that discard either side fail the integration checks.
  • The final repair suite passes 207 focused/catalog tests, including 46 actual-CLI payload cases with Bash output, parsed YAML and exact-byte comparisons. Independent re-review reproduced and closed the original scanner and filesystem failures, verified all 38 changed paths and retained all 24 critical preservation controls. The separate invalid-TSX-attribute subclaim did not reproduce and is not claimed as a fixed parser error.
  • CI, Integration and Shadscan pass. Actual browser jobs passed: 7 navigation checks, 13 production-gate tests, 3 Boneyard checks, 8 CMS tests, both 14-test smoke runs and demo auth preflight. All 33 conversations are resolved at the current readback. Hosted preview qualification remains outstanding pending the shared preview repair in fix(eve): stabilize preview builds and verification (AL-1913) #1915; these passes do not complete that requirement.

Deploy Checklist (for PRs to production or develop)

  • All current-head CI and applicable hosted preview checks pass
  • Base branch confirmed: develop; production release is separate
  • Deployment-discipline changes reviewed (N/A)
  • Migrations and environment additions reviewed (N/A)
  • Rollback: revert this focused catalog/tooling integration; no data migration
  • Canonical sources, refresh adapters and all generated mirrors verified
  • Final review conversations and merged predecessor preservation verified

RetriggerConfidence Score: 4/5

Do not merge until the hook overlay satisfies the repository's fail-fast requirement.

Findings

  1. P2 Make the hook fail fast ▶

Summary

The PR refreshes the skill catalog and its mirrors, strengthens refresh recovery and scanner adaptations, and adds a typed Contribution Operations command seam. A maintainer relying on the new git-guardrails overlay needs it to inspect commands safely; the overlay still lacks the repository-required fail-fast settings.

Reviews (4) · Last reviewed commit: "Merge branch 'develop' into cursor/skill..."

@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Shadscan score

Score: 29/100 (grade: F) — floor: 29

Scanned packages/ui with @shadscan/cli@0.1.1. Category breakdown and failing findings are in the job summary.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown

Walkthrough

Warning

Review details and warnings were omitted to fit the comment limit.

cursoragent and others added 2 commits September 28, 2026 22:49
Publish the audited lockfile catalog as signed Cursor Agent commits from
origin/develop. Keep OpenSpec at 1.9.0, Core overlays, and lockfile size 128.

Co-authored-by: Conrad O' <cobmojo@users.noreply.github.com>
The catalog refresh left agent-friction.md in the canonical Inngest
skill and left NestJS workflow and script copies in the runtime
mirrors. Regenerating the mirrors makes skills:verify match.
@cobmojo
cobmojo force-pushed the cursor/skills-catalog-signed-61e9 branch from 6027640 to efd9e6a Compare September 28, 2026 15:50
@cobmojo
cobmojo marked this pull request as ready for review September 28, 2026 15:50
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-28T20:58:15.349678Z da1f634 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pre-Mortem Bug Finder

Verdict: NOT SAFE TO MERGE (advisory comment; this review does not request changes).

Reviewed as a failure-mode hunt against develop at 6796c078a, not as a happy-path catalog refresh. The PR actually writes 128 lockfile-only and canonical skill trees, including Stripe well-known skills with no docs/ai/skills overlay, new Emil skills (animate-expo, ask-sonner, …), and tests that encode upstream bytes as the contract.

Hidden failures that matter after merge are agent-instruction bugs, not missing files: money-path Stripe skills now instruct a newer API/SDK than the live pin, tax remediation is a broken link plus a hardcoded txcd_, and auto-invoked Emil skills stall on first load. Green skills:verify / toContain tests prove the upstream text is present; they do not prove it is safe to follow.

I left separate inline comments for each confirmed or high-confidence issue. Babysit fail-closed overlay and the retired Inngest agent-friction drop look contained.

Open in Web View Automation 

Sent by Cursor Automation: Pre-Mortem Bug Finder

Comment thread .agents/skills/stripe-best-practices/SKILL.md Outdated
Comment thread .agents/skills/stripe-best-practices/SKILL.md Outdated
Comment thread .agents/skills/stripe-best-practices/references/tax.md Outdated
Comment thread tests/unit/docs/skills-lock-current-paths.test.ts Outdated
Comment thread scripts/refresh-upstream-skills.mjs
Comment thread docs/ai/skills/animate-expo/SKILL.md Outdated
Comment thread docs/ai/skills/improve-animations/SKILL.md Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shadcn/UI Review

Reviewed from the perspective of shadcn/ui correctness and the Maia design language. I left separate inline comments for each confirmed overlay teaching issue.

1. FINAL VERDICT

SAFE TO MERGE WITH FIXES

No product UI, theme tokens, components.json, or installed shadcn source changed. Product fail-conditions (wrong base API in apps, missing overlay titles, broken FieldGroup, fake Button isLoading, raw colors in product UI) do not fire. Overlay teaching on toast routing, cn imports, and Radix Slot got worse than develop and should be restored on a follow-up, not treated as a merge blocker for this signed catalog refresh.

2. EXECUTIVE SUMMARY

  1. What the PR is doing: Signed lockfile catalog refresh (128 upstreams) onto develop so git-attribution CI passes, plus a cleanup commit that drops retired Inngest agent-friction.md and NestJS pack bloat, then regenerates skill mirrors.
  2. What it gets right: Scope stays in skills/docs/scripts/tests. Core banners that say Base UI / render / no Radix still exist on several SKILL files. No Maia token or packages/ui primitive churn.
  3. Biggest shadcn / Maia risks: Agents will copy Base toast.add from @/components/ui/toast, import { cn } from "cn", and @radix-ui/react-slot asChild — none of which match this repo’s installed catalog or base-maia contract. The components-build index table lost the Core render wording that develop had.
  4. What matters most: Keep this PR mergeable as a catalog/lockfile move; restore CORE overlays on .agents/skills/shadcn and canonical docs/ai/skills/components-build so the next UI PR does not install the wrong toast or Slot.

3. PROJECT CONTEXT SNAPSHOT

Live bunx --bun shadcn@latest info --json from packages/ui:

  1. packageManager: bun@1.3.14
  2. framework: Manual (apps are Next.js App Router; the UI package is the shadcn root)
  3. isRSC: false (rsc: false in components.json)
  4. aliases: components @/components, utils @/lib/utils, ui @/components/shadcn
  5. style: base-maia (preset bc5ed0K, maia / zinc / Figtree / default radius) — this is the Maia target, not a mismatch
  6. base: base (Base UI render, not Radix asChild)
  7. iconLibrary: lucide
  8. tailwindVersion: v4
  9. tailwindCssFile: packages/ui/styles/globals.css
  10. Installed components relevant here: sonner, alert, empty, badge, separator, skeleton, button, card, dialog, sheet, drawer, field / FieldGroup, input, input-group, input-otp, command, tabs, avatar. Not installed: toast primitive, combobox, chat primitives (message-scroller, message, bubble, attachment, marker).

4. PR IMPACT MAP

  1. What changed: ~2539 files under .agents/skills, generated .claude / .cursor mirrors, docs/ai/skills, plus lockfile/scripts/tests. Head efd9e6a4 vs develop 6796c078.
  2. Shadcn components touched: None in product. Skill text now documents Base toast, chat primitives, and Radix Slot.
  3. Components that should have been used in examples: Installed sonner for toasts; project cn via @/lib/utils; Base render instead of Slot.
  4. Shared primitives: Not modified (packages/ui/components/shadcn/* untouched).
  5. Theme tokens / styling system: No edits to globals.css or components.json.
  6. Maia direction: Product UI unchanged (neutral). Skill examples drift toward generic upstream (Base Toast + Radix Slot + from "cn"), which is away from Core Maia practice if agents follow them.

5. HARD BLOCKERS

None. Skills-only refresh. No invalid product base API, missing Dialog/Sheet/Drawer titles, broken FieldGroup, incorrect InputGroup, fake Button loading props, or theme file splits.

6. HIGH RISK ISSUES

See inlines (each is its own thread):

  1. Base toast always-on rule — .agents/skills/shadcn/SKILL.md:67-69 — uninstalled toast primitive vs installed sonner.
  2. toast.add + @/components/ui/toast — .agents/skills/shadcn/rules/composition.md:96-100 — wrong alias and API for Core.
  3. import { cn } from "cn" — .agents/skills/shadcn/rules/styling.md:154 — not @/lib/utils → cnfast.
  4. Radix Slot / asChild example — docs/ai/skills/components-build/rules/as-child.md:23-27 — forbidden primitive in this repo.
  5. As-Child index row reverted — docs/ai/skills/components-build/SKILL.md:70 — develop overlay “this repo: Base UI render” was lost.

7. MEDIUM RISK ISSUES

  1. Uninstalled chat primitives taught as always-on — .agents/skills/shadcn/rules/chat.md and SKILL.md Chat section. MessageScroller / Bubble / Attachment / Marker are not in shadcn info. Teaching them without “not installed; do not add unless requested” will spawn custom chat UI or a surprise shadcn add.
  2. Shimmer / scroll-fade utilities — .agents/skills/shadcn/rules/styling.md new section. Tied to chat components Core does not ship. Risk of one-off animation CSS outside globals.css.
  3. components-build styling notes still mention clsx / tailwind-merge in places develop had pointed at cnfast. Same overlay-loss class as the Slot table.

8. LOW RISK ISSUES AND SUGGESTIONS

  1. .agents/skills/shadcn-ui/examples/auth-layout.tsx is example-only: @/components/ui/* aliases, Label+div forms not FieldGroup. Do not treat as product. A later overlay could retarget aliases and FieldGroup.
  2. .agents/skills/ask-sonner/SKILL.md still has !text-red-900 (raw color). Pre-existing / gated; do not block this PR.
  3. .agents/skills/pick-ui-library/SKILL.md still mentions cmdk / input-otp / zustand. Overlay + disable-model-invocation already constrain it.
  4. Do not comment generated .cursor / .claude mirrors; edit .agents/skills/shadcn and canonical docs/ai/skills/*, then bun run skills:sync.

9. MAIA FIT ASSESSMENT

  1. Does the changed UI feel like Maia? There is no changed product UI. Installed base-maia tokens are intact.
  2. Where it aligns: Catalog still describes Maia as a named preset; Core components.json stays maia/zinc/Figtree.
  3. Where it drifts: Agent-facing examples (Toast primitive, Slot, from "cn", chat density utilities) are generic upstream, not soft token-driven Core Maia.
  4. Acceptable? Yes for merge of a signed catalog. Not acceptable as the standing CORE overlay. Restore overlays so Maia work does not pick up Radix sharpness or a second toast system.

10. WHAT THE PR GETS RIGHT

  1. Does not fork or restyle shared shadcn primitives.
  2. Does not add Radix to packages/ui.
  3. Does not touch theme CSS or invent a second globals file.
  4. Leaves product sonner / Field / Alert / Empty / overlay titles alone.
  5. Composition.md still keeps the Sonner example for Radix/Aria after the new Base block.
  6. components-build SKILL.md banner still says Base UI / render / never add radix — the table row just stopped matching that banner.

11. ORDERED FIX PLAN FROM FIRST TO LAST

  1. Restore toast overlay on .agents/skills/shadcn/SKILL.md and rules/composition.md (sonner + @/components/shadcn). Why now: highest copy-paste blast radius for the next UI PR. Unlocks safe agent toast work.
  2. Restore cn overlay on rules/styling.md (@/lib/utils). Why now: every className merge example depends on it.
  3. Fix As-Child overlay on docs/ai/skills/components-build/SKILL.md:70 and rules/as-child.md (Base render, no Slot). Why now: prevents radix imports. Unlocks correct trigger composition.
  4. Gate chat.md / shimmer with “not installed”. Why now after API overlays: lower urgency, still stops surprise shadcn add.
  5. bun run skills:sync then skills:verify. Last among overlay edits so mirrors match canonical sources.

This order is API/alias/base-correctness first, then uninstalled-catalog teaching, then generated mirrors. Taste and preset cleanup wait.

12. VALIDATION PLAN BEFORE MERGE

This PR can merge as skills/lockfile. Before the overlay follow-up is considered done:

  1. Re-run bunx --bun shadcn@latest info --json in packages/ui (already: base-maia, base, lucide, sonner, no toast).
  2. bunx --bun shadcn@latest docs toast sonner button dialog — Base toast is toast.add; Core still uses sonner.
  3. Confirm touched product components remain installed; none were added/removed here.
  4. Confirm examples use @/components/shadcn and @/lib/utils, not @/components/ui or from "cn".
  5. Confirm skills say render / nativeButton={false}, not asChild / Slot, for this base.
  6. Forms: no product FieldGroup change; example auth-layout remains follow-up.
  7. Overlays: no product Dialog/Sheet/Drawer files in the diff.
  8. Buttons: no fake isLoading / isPending on shadcn Button (auth-layout local state + disabled only).
  9. Icons: lucide unchanged; no product icon PR.
  10. Theme: packages/ui/styles/globals.css untouched.
  11. Maia visual check: N/A for product; overlay restore is textual.
  12. No raw Tailwind colors or dark: overrides in product UI (none shipped). ask-sonner !text-red-900 remains a later overlay.

13. WHAT TO WATCH IN RE REVIEW

  1. Closest second look: the five inlined files, plus .agents/skills/shadcn/rules/chat.md.
  2. Human visual: none required for this PR (no rendered UI). For the overlay follow-up, read the restored toast and trigger examples, not a screenshot.
  3. Structural: CORE overlay still present after the next catalog refresh; skills:verify green; no radix dependency in packages/ui.

14. FOLLOW UP IDEAS

  1. Durable overlay tests so catalog refreshes cannot drop “Toast via sonner”, @/lib/utils, or Base render rows.
  2. Retarget shadcn-ui example aliases to @/components/shadcn and FieldGroup when someone next touches that pack.
  3. Gate pick-ui-library cmdk/zustand and ask-sonner raw colors (already invocation-gated).

15. OPEN QUESTIONS

  1. Whether Core will ever install official Base toast alongside sonner — current catalog and call sites say no.
  2. Whether chat primitives are an intentional upcoming install; they are not present now.
Open in Web View Automation 

Sent by Cursor Automation: Shadcn UI Review

Comment thread .agents/skills/shadcn/SKILL.md Outdated
Comment thread .agents/skills/shadcn/rules/composition.md Outdated
Comment thread .agents/skills/shadcn/rules/styling.md Outdated
Comment thread docs/ai/skills/components-build/rules/as-child.md Outdated
Comment thread docs/ai/skills/components-build/SKILL.md Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thermo-Nuclear Code Quality Review

Reviewed from the perspective of thermo-nuclear code quality and clean-code maintainability. I left separate inline comments for each confirmed issue.

Verdict

This PR has blocking code-quality issues (two High). No product-runtime P0 (auth, tenant, money, webhook, or data-boundary) was found. The second commit correctly dropped retired Inngest note and NestJS pack CI/scripts bloat; it does not address the two Highs below.

Findings

Finding 1: Duplicated secret-scanner annotator pushes sync-agent-skills.mjs past 1k lines

Severity: High
Location: scripts/sync-agent-skills.mjs:793 (copy at scripts/refresh-upstream-skills.mjs:1304)
Required change: Extract one shared module and import it from both scripts so sync-agent-skills.mjs drops back under 1000 lines and the two copies cannot drift.

Technical explanation:
Base scripts/sync-agent-skills.mjs was 842 lines. This PR adds ~200 lines of SECRET_SCANNER_* constants plus secretScannerComment / annotateSecretScannerLine / annotateSecretScannerMentions / annotateSecretScannerMentionsInTree, and a new listFilesRecursively. The same annotator is pasted into refresh-upstream-skills.mjs, which already had listFilesRecursively at ~1977 for tree hashing. Drift is already visible: sync's tree walker has a pathExists guard; refresh's does not. Thermo-nuclear rule 1 (do not cross 1k without decomposition) and rule 6 (reuse the canonical helper) both fail. Tests in tests/unit/script-verifiers.test.ts prove the behavior is shared, which is exactly why one module should own it.

Plain-language explanation:
The same “add a secret-scanner footnote to demo tokens” helper was copied into two already-large scripts. One of those scripts just crossed the 1000-line line. Two copies will get out of sync (they already differ on a missing-directory check).

Impact:
Future scanner/pragma fixes must be edited twice or they silently diverge. sync-agent-skills.mjs is now harder to review and more likely to keep growing. This is a maintainability blocker, not a production-data bug.

Suggested fix:
Add scripts/lib/annotate-secret-scanner.mjs (same shared-script pattern as scripts/cms/lib/). Export the constants and annotateSecretScannerMentionsInTree. Import from both sync-agent-skills.mjs and refresh-upstream-skills.mjs. Reuse refresh's existing listFilesRecursively instead of adding a second walker in sync. Keep the existing vitest coverage; do not duplicate tests.

Finding 2: Newly canonical components-build now teaches Radix asChild / @radix-ui/react-slot

Severity: High
Location: docs/ai/skills/components-build/AGENTS.md:1026 (also rules/as-child.md:24 and SKILL.md category 9)
Required change: Do not leave Radix Slot / asChild as agent-facing canonical guidance. Rewrite or omit those files, restore the Core wording that was already in SKILL.md on develop, and add a not.toContain test so the next catalog refresh cannot bring it back.

Technical explanation:
On develop, canonical docs/ai/skills/components-build/ was overlay-bearing SKILL.md only. Category 9 said “Slot composition (this repo: Base UI render)”. This PR vendors the full upstream tree (AGENTS.md 2176 lines, rules/as-child.md with import { Slot } from "@radix-ui/react-slot") and reverts the SKILL.md table/quick-ref to “Radix Slot composition pattern”. Overlay still says never add @radix-ui/* and use Base UI render. docs/ai/rules/frontend.md:30-31, packages/ui/AGENTS.md, and references/upstream.md (“no Radix / asChild”) agree. The same PR already rewrites Emil asChild via required POST_REFRESH_REPLACEMENTS and asserts it in tests/unit/docs/emilkowalski-skills.test.ts. skill-quality-gate.test.ts only checks SKILL.md overlay headings, so this tree is untested. Codex/Cursor load AGENTS.md as always-on; overlay on SKILL.md does not win.

Plain-language explanation:
The repo's UI rule is “Base UI only — no Radix, no asChild.” This refresh copied the upstream components-build book, which is a Radix asChild tutorial, into the official skill folder agents actually read. The warning sticker on SKILL.md does not stop that. We already fixed this class of leak for Emil skills in this same PR; components-build was left as the raw upstream copy.

Impact:
Agents following canonical skills will propose asChild, Radix Slot, and @radix-ui/react-slot in packages/ui and app UI. That violates a standing Core invariant and will produce rejected or regressive UI PRs. This is an agent-instruction contract failure, not a runtime crash.

Suggested fix:
Pick one:

  1. Preferred: Same treatment as Emil component-design.md — rewrite AGENTS.md §9 and rules/as-child.md to Base UI render / buttonVariants on Link, and restore SKILL.md category 9 / quick-ref to the develop Core wording.
  2. Simpler judo: Do not promote AGENTS.md or rules/as-child.md into canonical; keep overlay SKILL.md as the canonical surface (develop's model) and prune those files from mirrors on sync.
    Then add expect(source).not.toContain("asChild") / not.toContain("@radix-ui/react-slot") for the components-build canonical tree (same assertion already used for Emil).

Validation

  • bunx vitest run (10 skill-related files): 10 files, 102 tests passed.
  • bun run skills:verify: Skill mirrors match canonical sources.
  • Targeted files: tests/unit/docs/babysit-skill.test.ts, design-skill-packs.test.ts, emilkowalski-skills.test.ts, git-guardrails-skill.test.ts, grill-for-unknowns-skill.test.ts, skill-quality-gate.test.ts, skills-lock-current-paths.test.ts, agent-instruction-routing.test.ts, tests/unit/script-verifiers.test.ts, tests/unit/scripts/inngest-skill-references.test.ts.
  • These gates passing does not disprove the findings: the annotator is covered as duplicated behavior, and components-build Radix guidance is not asserted.

What I checked

  • Full PR vs develop 6796c078a at HEAD efd9e6a4a (two commits; lockfile 128 skills).
  • scripts/sync-agent-skills.mjs 842→1089 and scripts/refresh-upstream-skills.mjs annotator duplication / listFilesRecursively overlap.
  • Canonical vs mirror components-build trees; overlay vs body; POST_REFRESH_REPLACEMENTS Emil asChild rewrite; frontend.md Base UI contract; references/upstream.md.
  • Secret-scanner pragma tests (including JSON skip and three-root mirroring).
  • Second-commit NestJS .github/scripts deletions and Inngest agent-friction.md removal.
  • Prior related reviews on #1902 / #1904 (same two Highs still present on this HEAD).

Notes

  • Overlay-stripped SKILL.md vs lockfile hash: not a bug (lock hashes raw upstream).
  • Files already ≥1k before this PR (refresh-upstream-skills.mjs was 2092): not flagged for size alone.
  • .prettierignore of nestjs/playwright/payload reference trees: keeps pragma on the same line as demo tokens; not a scanner bypass.
  • Annotating targetRoots[0] then mirroring: tests cover all three roots.
  • JSON demo tokens left without pragma: invalid JSON if annotated; tests document this.
  • NestJS leftover always-on AGENTS.md is still large after the bloat trim; pre-existing, not a new blocker from this PR.
  • docs/ai/skills/components-build is not driven by skills:refresh-upstream today, so Emil-style POST_REFRESH will not fire unless this pack is added to that pipeline or rewritten in the catalog copy step.
  • No product UI/runtime change; browser verification N/A.
Open in Web View Automation 

Sent by Cursor Automation: Thermonuclear Cursor Code Review

Comment thread scripts/sync-agent-skills.mjs Outdated
Comment thread docs/ai/skills/components-build/AGENTS.md Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Improve Codebase Architecture Review

Verdict

This PR improves catalog architecture. It deepens the signed 128-upstream freeze, stamps Grill-style explicit discovery on write-swift and mobile-native, strips NestJS .github and scripts from the lock, and retires the Inngest agent-friction note. It still ships three high-confidence issues that keep discovery and runtime instruction seams leaky: High animate-expo auto-discovery, Medium obra TDD YAML versus overlay split, High leftover NestJS AGENTS.md always-on at runtime mirrors.

Reviewed from the perspective of module depth, locality, and AI navigability. Separate inline comments are attached to each confirmed issue so they can be resolved independently.

Architectural Findings

Finding 1: animate-expo discovery seam stays auto while overlay is Expo-only

Severity: High
Location: docs/ai/skills/animate-expo/SKILL.md lines 1-4; scripts/refresh-upstream-skills.mjs EMIL_EXPLICIT_ONLY_SKILLS lines 63-70; tests/unit/docs/emilkowalski-skills.test.ts sibling expects around 143-160
Architectural concern: The new animate-expo module YAML interface auto-matches Core web motion (sheets, press, transitions) while the CORE-OVERLAY implementation says Expo-only. The refresh adapter that stamps disable-model-invocation: true for unused Emil skills omits this name, so the next refresh will keep the leak. Tests freeze the flag on animate, write-swift, and mobile-native, not animate-expo.
Required change: Add disable-model-invocation: true to canonical animate-expo YAML. Add animate-expo to EMIL_EXPLICIT_ONLY_SKILLS so ensureEmilDisableModelInvocation owns the stamp. Add the same expect() beside the sibling lock in emilkowalski-skills.test.ts. Run bun run skills:sync.

Technical explanation:
Depth lives in ensureEmilDisableModelInvocation: one Set membership hides YAML mutation. animate-expo was added to emilKowalskiSkillNames but not to that Set, so the adapter is a no-op for the one unused skill whose description collides with Core web motion. The YAML interface is almost as wide as animate, while the overlay hides Expo-only ownership. Callers must already know implementation details to avoid loading it. That is a shallow discovery module with negative leverage on Next.js surfaces.

Plain-language explanation:
Cursor will auto-load a React Native animation skill when someone mentions sheets or press feedback on admin, donor, missionary, or packages/ui. Core has no Expo app. The overlay already says do not do that, but the machine-readable frontmatter still invites it, and the refresh script will not stamp the flag on the next catalog pass.

Architectural impact:
Cognitive load and wrong-stack recipes leak into every web motion task. AI navigability gets worse because two animation modules compete on the same verbs. The leak has now survived catalog refreshes tracked on PRs 1430, 1902, and 1904.

Suggested deepening opportunity:
Keep one Emil explicit-only adapter. Membership in EMIL_EXPLICIT_ONLY_SKILLS should be the whole unused-skill discovery policy. Do not add a second helper. Do not split the refresh script.

Finding 2: obra TDD YAML auto-invokes while overlay claims explicit companion

Severity: Medium
Location: docs/ai/skills/test-driven-development/SKILL.md lines 1-24; tests/unit/docs/design-skill-packs.test.ts explicitOnlySkills omits the name
Architectural concern: This PR newly canonicalizes obra Superpowers TDD. YAML description still says use it when implementing any feature or bugfix. The overlay says ordinary Core implementation uses docs/ai/skills/tdd/SKILL.md automatically, and obra only when explicitly requested. Two TDD modules now present competing auto-invoke interfaces.
Required change: Stamp disable-model-invocation: true on obra YAML through the obra refresh path, matching Grill unused companions. Add test-driven-development to explicitOnlySkills. Keep Core tdd as the auto skill with skip rules for docs, format, mirrors, and provenance.

Technical explanation:
YAML is the public discovery interface. The overlay is implementation. When they disagree, callers need both. Core tdd is the deep module: repo workflow, seams, and skip rules. obra is valuable as iron-law examples behind an explicit seam. Auto YAML on the companion fragments TDD locality. design-skill-packs.test.ts checks overlay text and omits this name from explicitOnlySkills, so the split is frozen.

Plain-language explanation:
Agents may load two TDD skills at once. One says skip fake tests for catalog work. The other says TDD for any feature. That is how a skills-lock PR attracts a fake failing test.

Architectural impact:
TDD ownership splits. Catalog and mirror work become more likely to get artificial RED tests, against the Core tdd overlay. Maintainers cannot trust the frontmatter without also reading the overlay.

Suggested deepening opportunity:
One auto TDD module (tdd). One explicit companion (obra) whose YAML cannot auto-match. Same stamp shape as Grill, not a new abstraction.

Finding 3: NestJS pack-root AGENTS.md leftover stays always-on

Severity: High
Location: .agents/.cursor/.claude skills/nestjs-best-practices/AGENTS.md leftover; docs/ai/skills/nestjs-best-practices/references/upstream.md line 18; tests/unit/docs/skills-lock-current-paths.test.ts lines 94-103; scripts/sync-agent-skills.mjs pruneVendoredSkillJunk
Architectural concern: Canonical nestjs-best-practices has no AGENTS.md. This PR documents do not copy pack-root AGENTS.md, .github, or scripts, and tests freeze .github and scripts as absent. Runtime mirrors still keep a leftover AGENTS.md of about 6000 lines. Cursor loads it as always-on NestJS doctrine. pruneVendoredSkillJunk only deletes Archive.zip, __MACOSX, and .DS_Store. overlayDirectory preserves extras, so CLI add leftovers survive sync.
Required change: Delete the leftover AGENTS.md mirrors (plus unused README or .gitignore if present). Extend this test to expect AGENTS.md absent at canonical and runtime roots. Deepen prune or sync so pack-root files missing from canonical cannot be restored by npx skills add.

Technical explanation:
Pack-root AGENTS.md fails the deletion test as a shallow leftover: deleting it concentrates NestJS into the SKILL.md overlay instead of exploding callers. Leaving it always-on leaks NestJS modules, controllers, and TypeORM into every agent session, against the overlay that Core is not a NestJS application. The test interface is narrower than the invariant upstream.md just documented, so the seam will regress on the next CLI add.

Plain-language explanation:
The PR cleaned NestJS GitHub workflows and helper scripts, then left a huge NestJS handbook in a folder Cursor always reads. Agents can start writing Nest controllers inside a Bun Next.js monorepo.

Architectural impact:
Always-on cognitive load, domain-language drift (Nest modules versus Core packages/api), and a real path to introducing a forbidden runtime. This is not a re-litigation of the NestJS overlay text. The leftover pack-root file is a distinct ownership failure.

Suggested deepening opportunity:
Make pruneVendoredSkillJunk (or the sync overlay) own pack-root cleanup: if a file exists at a runtime skill root and not in canonical, delete it. Tests should lock that interface. SKILL.md overlay remains the one NestJS module.

Deletion test observations

  • animate-expo YAML without an explicit-only stamp: deleting the auto-matching verbs, or stamping disable-model-invocation: true, concentrates Expo ownership into the overlay. Leaving the verbs auto-invoked explodes Expo recipes across web callers. The current YAML is shallow at discovery.
  • EMIL_EXPLICIT_ONLY_SKILLS plus ensureEmilDisableModelInvocation: deleting that adapter would scatter YAML stamps across skills. It is a deep adapter. animate-expo simply is not a member yet.
  • obra test-driven-development YAML auto description: deleting auto-invoke concentrates TDD into Core tdd. Keeping both auto splits the interface.
  • Runtime nestjs AGENTS.md: deleting it concentrates NestJS into SKILL.md. Keeping it always-on is leftover pack machinery, not a second deep module. pruneVendoredSkillJunk as written is too shallow (junk names only) relative to the pack-root invariant this PR documented.
  • Generated 3-way mirrors, the 128-name freeze, lock hashes, better-* auto with Base UI overlays, ask-sonner toaster rewrite, apple-design auto, Resend skill versus CLI version, and inventing a fake TDD RED for this catalog PR: not findings. Several of those were reviewed and disproved or marked pre-existing.

Validation

  • bun x vitest run tests/unit/docs/emilkowalski-skills.test.ts tests/unit/docs/skills-lock-current-paths.test.ts tests/unit/docs/design-skill-packs.test.ts tests/unit/script-verifiers.test.ts: 4 files, 21 passed, 338ms
  • bun run skills:verify: Skill mirrors match canonical sources.

What I checked

  • Canonical versus generated skill ownership (docs/ai/skills, .agents, .cursor, .claude)
  • Discovery seam disable-model-invocation versus CORE-OVERLAY triggers
  • Emil refresh adapter EMIL_EXPLICIT_ONLY_SKILLS and ensureEmilDisableModelInvocation
  • Grill explicit-only stamp as the deeper adapter to copy, not replace
  • obra TDD canonicalization versus Core tdd overlay skip rules
  • NestJS canonical SKILL.md overlay, references/upstream.md pack-root rule, runtime AGENTS.md leftover, pruneVendoredSkillJunk, overlayDirectory extra preservation
  • skills-lock-current-paths and emilkowalski tests as the test interface of those seams
  • CONTEXT.md, docs/adr, docs/ai/protected-paths.md, docs/ai/rules/agent-skill-routing.md, openspec/specs/agent-instruction-system/spec.md, scripts/AGENTS.md
  • Prior catalog reviews on PRs 1430, 1902, and 1904 (do not re-flag items listed there except animate-expo, which this HEAD still ships)

Notes

  • TDD overlay for this PR: catalog, lockfile, overlay, and mirror provenance work must not invent a fake RED test. Characterization tests at the discovery seam (YAML flag, Set membership, AGENTS.md absence) are the right next tests if this PR is revised.
  • HTML deepening report: /tmp/architecture-review-20260928-155628.html (also /opt/cursor/artifacts/architecture-review-20260928-155628.html). Top recommendation is Finding 1.
  • No product code was changed by this review.

Action Directives

  1. Stamp animate-expo YAML with disable-model-invocation: true and add the name to EMIL_EXPLICIT_ONLY_SKILLS with a sibling expect().
  2. Stamp obra test-driven-development YAML the same way and add it to explicitOnlySkills. Keep Core tdd as the auto module.
  3. Delete leftover nestjs-best-practices AGENTS.md mirrors and freeze that absence in skills-lock-current-paths.test.ts. Deepen pack-root prune so CLI add cannot restore them.
Open in Web View Automation 

Sent by Cursor Automation: Improve Codebase Architecture PR Review

Comment thread docs/ai/skills/animate-expo/SKILL.md
Comment thread scripts/refresh-upstream-skills.mjs
Comment thread docs/ai/skills/test-driven-development/SKILL.md
Comment thread tests/unit/docs/skills-lock-current-paths.test.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Critical Bug Check

Verdict: no critical bugs found. This pull request can merge from a product-safety standpoint. I did not open a follow-up fix.

Plain language

This change refreshes the agent skills catalog (the instruction packs coding agents read) and the lockfile that pins those packs to known content hashes. It does not change the donor, missionary, or admin apps, and it does not touch payments, auth, or the database.

In everyday terms: this is a documentation and tooling update for how AI assistants work in the repo. It does not change what customers see, how money is handled, or who can access data. I looked for anything that could still blow up a production flow, leak access, or silently destroy real product data. I did not find that class of bug.

What I inspected

Compared 6796c078a...efd9e6a4a (commits 16f5aec56, efd9e6a4a). The only executable repo-owned paths are skill-maintenance scripts and their tests:

  • scripts/refresh-upstream-skills.mjs — local + GitHub refresh, atomic swap, EXDEV copy/replace, lock hashes
  • scripts/sync-agent-skills.mjs — junk prune (Archive.zip / __MACOSX / .DS_Store / ._*), secret-scanner allowlist on .agents/skills only
  • scripts/refresh-inngest-skills.mjs — retired agent-friction.md delete, new inngest-api-cli / REST v2 references
  • skills-lock.json — SHA-256 content hashes (computedHash, optional treeHash), not cryptographic signatures; +18 skills, none removed
  • Overlays: git-guardrails fail-closed hook; babysit sdkVersion 6.0.3 with no latest fallback in the Core overlay

Zero diffs under apps/, packages/, supabase/, or root .github/workflows/.

Technical findings

No P0/P1 issues. Traced and cleared:

  • Path safety: hardcoded skill slugs; assertSafeCanonicalSkillDirName / assertSafeRelativePath / assertPathInside on GitHub extras and companions; Inngest rm is hardcoded docs/ai/skills/inngest-api/references/agent-friction.md.
  • Swap / EXDEV: dest is removed before fs.cp so trees do not merge leftovers; restore uses AggregateError if backup restore fails; focused missing directory still fail-closes before any canonical swap.
  • GitHub missing SKILL.md: prepareGithubSkillRefresh returns null and skips; canonical copy is left alone.
  • Command injection: spawnSync with argv arrays, shell: false.
  • Workflows: NestJS .github/workflows copies exist only inside skill trees in the first commit and are deleted at HEAD. GitHub Actions only runs repo-root .github/workflows/.
  • Auto-invocation: frontend-design, design-taste-frontend, redesign-existing-projects, interface-review, emil-prototype, and animate keep disable-model-invocation: true.
  • Scanner annotation: constructed demo token; writes only .agents/skills, then mirrored. Canonical docs/ai/skills is not patched. JSON is left unpatched.
  • Tests: coverage expanded (Emil clone hashes, overlays, Inngest retirement). The animation-vocabulary fence assertion was rewritten equivalently (toHaveLength(4)), not weakened.

Rejected as a merge blocker: local prepareSkillRefresh (scripts/refresh-upstream-skills.mjs) still only access()es the source directory. Jakub Krehel and test-driven-development have no required SKILL.md read, so a corrupt install (directory present, SKILL.md missing) could replace docs/ai/skills/<name>/ on skills:refresh-jakubkrehel or an unfocused refresh of that group. That is maintainer-only, git-recoverable, and needs an abnormal tree. GitHub clones already skip this case. Same fail-open already existed for other local sources (for example npm-deps-cleanup). Not product data loss, auth, or a customer-facing crash.

No inline comments: there is no confirmed critical defect to pin on a diff line.

Open in Web View Automation 

Sent by Cursor Automation: Critical Bug Finding

@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: efd9e6a4a5

ℹ️ 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/refresh-upstream-skills.mjs Outdated
Comment thread docs/ai/skills/playwright-best-practices/advanced/network-advanced.md Outdated
Comment thread docs/ai/skills/playwright-best-practices/testing-patterns/graphql-testing.md Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug Finder v2

1. FINAL VERDICT

NOT SAFE TO MERGE

CI is green because skills:verify only checks that skill mirrors match each other, and new tests assert the refreshed Stripe catalog date. That is a false green. Confirmed agent-contract bugs landed in wizard, shadcn, obra TDD, and Stripe tax guidance.

2. EXECUTIVE SUMMARY

What the PR changes: Refresh of skills-lock.json (128 upstreams) plus mirrored skill trees, refresh scripts, overlay restore for a few Core-hardened skills, and tests that lock the new catalog markers.

What is already broken: Develop’s wizard disable-model-invocation: true is gone and the YAML now auto-matches credentials/CI secrets while template.sh writes .env and gh secret set. Shadcn now teaches Base UI toast.add from a missing @/components/ui/toast and import { cn } from "cn". New obra TDD YAML says use it for every feature/bugfix, contradicting its own overlay. Stripe skills teach 2026-07-29.dahlia against live 2026-05-27.dahlia. New tax.md hardcodes a txcd_ after telling agents never to do that, and links undefined#choosing-a-product-tax-code.

What matters most: Restore Core frontmatter/overlays at the refresh source, then add tests that would fail if those overlays are wiped again. Do not treat skills:verify or current unit tests as proof these skills are Core-safe.

Technical: Lockfile-only GitHub/well-known installs overwrite .agents/skills/<name> with upstream. Only ask-sonner toaster, a subset of Emil flags, and git-guardrails get post-refresh restore. Wizard/shadcn/Stripe skip that path. Sync copies the unsafe .agents trees into .cursor and .claude. Verify then passes.

Plain: This PR copied newer vendor skill docs into the repo. A few Core safety notes were put back; the rest were not. Agents will now follow the vendor copy, including secret-writing and broken UI/Stripe recipes.

3. REPO AND PR DEBUG CONTEXT

  • Stack: Bun 1.3.14 + Turborepo; three Next.js App Router apps; shared packages/api, packages/ui (base-maia / Base UI / Zinc). Skills canonical tree docs/ai/skills/ mirrored to .agents, .cursor, .claude.
  • HEAD: efd9e6a4a vs develop 6796c078a. Branch cursor/skills-catalog-signed-61e9.
  • CI on this SHA: format, lint, typecheck, test-unit, build, smoke, e2e-smoke, integrity, audit, ci-gate SUCCESS. Mergeable, review-blocked.
  • Local: bun run skills:verify passed (“Skill mirrors match canonical sources.”). Focused vitest: 4 files / 21 tests passed, including the test that requires Stripe 2026-07-29.dahlia.
  • High-risk systems touched: agent routing YAML, secret-writing wizard, UI generation recipes, Stripe API/tax guidance, skill refresh/verify pipeline.
  • Assumptions that changed: lockfile hashes are treated as the Core-safe skill contents; skills:verify is treated as overlay verification. Neither is true for lockfile-only skills.
  • Unchanged consumers affected: any agent using .agents/.cursor/.claude wizard, shadcn, stripe-best-practices, upgrade-stripe, test-driven-development; live Stripe pin in packages/api/src/stripe/api-version.ts; toaster in packages/ui/components/shadcn/sonner.tsx; cn via packages/ui/lib/utils.ts → cnfast.
  • Not a new bug vs current develop: git-guardrails fail-closed overlay + executable hook; ask-sonner Core toaster import. Those were #1904 issues and are restored here.
  • Title “signed”: skills-lock.json stores computedHash/treeHash, not signatures. Both commits are unsigned (%G? = N). Naming only; not a functional blocker.

4. CONFIRMED BUGS

4.1 Wizard lost auto-invoke guard and now matches secret setup

  • Files: .agents/skills/wizard/SKILL.md (mirrors .cursor/skills/wizard, .claude/skills/wizard); .agents/skills/wizard/template.sh; skills-lock.json wizard.computedHash bdf31d48…
  • Symptom: Frontmatter no longer has disable-model-invocation: true. Description auto-matches “setting up credentials or CI secrets”.
  • Root cause trace: Develop YAML had the flag and a narrower description. Lockfile refresh from mattpocock/skills skills/engineering/wizard/SKILL.md wrote upstream YAML into .agents/skills/wizard. There is no docs/ai/skills/wizard. scripts/refresh-upstream-skills.mjs never lists wizard, so applyPostRefreshReplacements never restores it. skills:sync mirrors the unflagged file. skills:verify only checks the three trees match. template.sh:26 defaults ENV_FILE=.env; :143-146 runs gh secret set; :199-201 writes Stripe keys.
  • Evidence: git show 6796c078:.agents/skills/wizard/SKILL.md has the flag; HEAD does not. Hash matches lock. No wizard tests analogous to tests/unit/docs/emilkowalski-skills.test.ts.
  • Why this PR: This refresh is what dropped the flag and widened the description.
  • Smallest safe fix: Persist Core wizard frontmatter through refresh (restore disable-model-invocation: true and a non-secret-matching description, as on develop). Put that restore on the lockfile-install path, not only refresh-upstream-skills.mjs sources.
  • Defense in depth: Canonical overlay under docs/ai/skills/wizard or a refresh replacement; unit test that wizard YAML keeps the flag and does not match “CI secrets”; fail skills:verify if the flag is missing.
  • Must fix before merge: yes (secret-writing auto-invoke).
  • Still verify: Claude Code respects disable-model-invocation; Cursor/Codex route on description. Both layers must be safe.

4.2 Shadcn teaches a toast module Core does not ship

  • Files: .agents/skills/shadcn/rules/composition.md:91-111
  • Symptom: Base UI recipe is import { toast } from "@/components/ui/toast" then toast.add({ title }).
  • Root cause trace: Develop taught import { toast } from "sonner". Upstream shadcn skill (skills-lock.json source shadcn-ui/ui) now branches Base UI vs Radix. Core components.json style is base-maia, ui alias @/components/shadcn. Live toaster is packages/ui/components/shadcn/sonner.tsx. No packages/ui/components/ui/toast and no toast.add. Lockfile-only shadcn is not overlay-restored.
  • Evidence: git show 6796c078:.agents/skills/shadcn/rules/composition.md “Toast notifications use sonner”; HEAD toast.add; ls packages/ui/components/ui/toast.tsx missing.
  • Smallest safe fix: Overlay-restore the Core toast recipe to sonner / @asym/ui/components/shadcn/sonner (same pattern as ask-sonner).
  • Defense: Test that composition.md does not contain toast.add or @/components/ui/toast.
  • Must fix before merge: yes (agents will generate broken UI).

4.3 Shadcn teaches import { cn } from "cn"

  • Files: .agents/skills/shadcn/rules/styling.md:154
  • Symptom: Correct example imports a package that is not in the workspace.
  • Root cause trace: Develop used @/lib/utils. Upstream now uses "cn". Core packages/ui/lib/utils.ts is export { cn } from "cnfast". packages/ui/package.json depends on cnfast, not cn. components.json aliases.utils is @/lib/utils.
  • Evidence: no from "cn" in packages/ui; cnfast is the real dependency.
  • Smallest safe fix: Overlay-restore import { cn } from "@/lib/utils" (or @asym/ui equivalent).
  • Defense: Test that styling.md does not contain from "cn".
  • Must fix before merge: yes.

4.4 Obra TDD YAML auto-invokes despite overlay saying explicit-only

  • Files: docs/ai/skills/test-driven-development/SKILL.md:1-4; mirrors; scripts/refresh-upstream-skills.mjs (obra source at 163-168, no YAML restore); tests/unit/docs/design-skill-packs.test.ts:94-99
  • Symptom: description: Use when implementing any feature or bugfix, before writing implementation code and no disable-model-invocation.
  • Root cause trace: Overlay body (lines 20-24) says ordinary Core work uses docs/ai/skills/tdd/SKILL.md and obra is explicit. Routers use YAML, not the overlay essay. Working restore for sibling explicit-only skills is string-replace injection (grill-for-unknowns, frontend-design, design-taste-frontend at 406-445). skills:refresh-obra-tdd has no such replace. explicitOnlySkills omits test-driven-development, so :208-214 never asserts the flag.
  • Evidence: HEAD YAML vs overlay contradiction; refresh replacements exist for other packs, not this one; 21 focused tests passed including design-skill-packs.
  • Smallest safe fix: Inject disable-model-invocation: true and narrow the YAML description in the obra refresh replacements; add the skill to explicitOnlySkills.
  • Must fix before merge: yes (will steal routing from Core TDD on every feature).

4.5 Stripe tax guide contradicts itself and ships a broken URL

  • Files: .agents/skills/stripe-best-practices/references/tax.md (new)
  • Symptom: Line 58: never hardcode txcd_. Line 72: hardcodes txcd_10103001 and links undefined#choosing-a-product-tax-code.
  • Root cause trace: Well-known Stripe docs landed as lockfile content. No Core overlay. Test :210-212 only asserts tax.md exists.
  • Evidence: file is new in this PR; fragment undefined# is not a valid docs URL.
  • Smallest safe fix: Overlay-restore remediation text to point at the in-file heading and remove hardcoded txcd_ examples, or drop the broken sentence.
  • Must fix before merge: yes for money-path agent guidance.

5. HIGH CONFIDENCE LIKELY BUGS

5.1 Stripe catalog pin 2026-07-29 vs live 2026-05-27

  • Files: .agents/skills/stripe-best-practices/SKILL.md:17; .agents/skills/upgrade-stripe/SKILL.md:7; packages/api/src/stripe/api-version.ts:13; packages/api/package.json stripe@22.2.0; tests/unit/docs/skills-lock-current-paths.test.ts:194-217
  • Why likely: Skills say “always use latest” 2026-07-29.dahlia. Live factory pin is 2026-05-27.dahlia (LatestApiVersion for stripe 22.2.0). Agents following the skill will bump STRIPE_API_VERSION / SDK without the changelog review the product pin exists to force.
  • Trace: Develop catalog was 2026-04-22.dahlia. Refresh moved catalog forward past the live pin. The new test requires 2026-07-29, so CI cannot fail.
  • Proof still needed: Runtime of existing apps is unchanged until an agent applies the skill. That is still a post-merge failure mode for this PR’s consumers (agents).
  • Likely fix: Overlay that names Core’s pin (2026-05-27.dahlia / stripe@22.2.0) and forbids unreviewed bumps; change the unit test to compare catalog vs STRIPE_API_VERSION instead of locking the vendor date.
  • Treat as merge-blocking for this catalog PR.

6. POSSIBLE ISSUES NEEDING EVIDENCE

  • animate-expo missing disable-model-invocation: omitted from EMIL_EXPLICIT_ONLY_SKILLS (scripts/refresh-upstream-skills.mjs:63-70). Description is Expo/RN-specific, so Core Next.js auto-invoke is less likely than wizard/TDD. Overlay already says web apps should use animate. Need a routing-log repro before calling it a blocker.
  • Lock vs on-disk hash drift on overlay/pragma skills: expected, not a verify failure. Do not “fix” by deleting overlays.

7. ARCHITECTURE QUESTIONS

The refresh pipeline has two classes of skills:

  1. Sources in scripts/refresh-upstream-skills.mjs with post-refresh restore (Emil flags, git-guardrails, ask-sonner toaster, some YAML injections).
  2. Lockfile-only GitHub/well-known skills (wizard, shadcn, Stripe) with no Core restore.

skills:verify only proves .agents / .cursor / .claude / docs/ai/skills mirrors match. It cannot see a missing Core overlay that was never in those trees.

Patching each wiped skill one by one will keep regressing on the next catalog refresh. The pattern is unsound until lockfile-only Core-hardened skills have the same restore+test seam as Emil/ask-sonner/git-guardrails.

8. WHAT THE PR GETS RIGHT

  • Git-guardrails hook is fail-closed again via scripts/refresh-overlays/git-guardrails-block-dangerous-git.sh and ensureGitGuardrailsFailClosed.
  • Ask-sonner toaster import is restored to @asym/ui/components/shadcn/sonner.
  • NestJS pack bloat / retired Inngest note cleanup reduces noise.
  • skills:verify still correctly checks mirror equality (that check is just the wrong gate for overlay safety).

9. ORDERED FIX PLAN (FIRST → LAST)

  1. Wizard frontmatter restore — secret-writing auto-invoke. Unlocks any later catalog work being safe to merge.
  2. Shadcn toast + cn overlays — stops broken UI generation. Same restore mechanism as ask-sonner.
  3. Obra TDD YAML + explicitOnlySkills — stops dual-TDD auto-routing.
  4. Stripe pin overlay + tax.md — money-path agent contract.
  5. Tests that fail if those overlays disappear — otherwise the next refresh repeats this PR.
  6. Architecture: one restore registry for lockfile-only Core-hardened skills. Do not start this until 1–5 are in, or the registry will be designed around the wrong examples.

Do not bump timeouts, skip tests, or “just re-run skills:verify”.

10. VALIDATION PLAN BEFORE MERGE

  • rg -n "disable-model-invocation" .agents/skills/wizard/SKILL.md must hit.
  • Wizard description must not include “CI secrets” / credential auto-match unless the flag is present and Cursor routing is proven safe.
  • rg -n "toast.add|from \"cn\"" .agents/skills/shadcn must be empty; toast recipe must be sonner/@asym/ui.
  • docs/ai/skills/test-driven-development/SKILL.md YAML must have disable-model-invocation: true and must not say “any feature or bugfix”.
  • Stripe skills must name 2026-05-27.dahlia or explicitly defer to STRIPE_API_VERSION; tax.md must not contain undefined# or hardcoded txcd_ as a required example.
  • Add failing tests first for wizard flag, shadcn toast/cn, obra explicit-only, Stripe pin vs packages/api/src/stripe/api-version.ts.
  • Re-run: those new tests, bun run skills:verify, focused vitest above, then bun run skills:refresh-obra-tdd / lockfile restore in a scratch copy to prove overlays survive.
  • Re-diff against develop (6796c078a), not this branch in isolation.

11. WHAT TO WATCH IN RE-REVIEW

  • Wizard YAML (both flag and description).
  • Whether restore runs on lockfile install, not only refresh-upstream-skills.mjs.
  • Shadcn composition/styling still matching Core base-maia + sonner + cnfast.
  • New tests actually failing if overlays are deleted.
  • Stripe overlay vs live pin, not vs “latest Stripe blog date”.

12. FOLLOW UP IDEAS (CAN WAIT)

  • Rename “signed lockfile” to hash-pinned catalog.
  • Consider animate-expo explicit-only if routing logs show web false-positives.
  • Broader restore registry for all Core-hardened lockfile skills.

13. OPEN QUESTIONS

  • Cursor/Codex description routing for wizard after flag restore: if description still matches secrets, flag-only is not enough.
  • Whether Stripe well-known skills should be Core-overlaid or dropped from auto-routing until the pin matches.

Inline comments are on the RIGHT-side hunks that introduced each bug. Resolve a thread only after the overlay/test for that file exists on HEAD.

Open in Web View Automation 

Sent by Cursor Automation: Bug Finder 2.0

Comment thread .agents/skills/wizard/SKILL.md
Comment thread .agents/skills/shadcn/rules/composition.md Outdated
Comment thread .agents/skills/shadcn/rules/styling.md Outdated
Comment thread docs/ai/skills/test-driven-development/SKILL.md
Comment thread .agents/skills/stripe-best-practices/SKILL.md Outdated
Comment thread .agents/skills/stripe-best-practices/references/tax.md Outdated
Comment thread scripts/refresh-upstream-skills.mjs
Comment thread tests/unit/docs/skills-lock-current-paths.test.ts Outdated
Comment thread tests/unit/docs/design-skill-packs.test.ts
Preserve both vendor routing lists and all source/refresh safeguards. Retain the 128-entry catalog while independently verifying the merged architecture skill and its exact reviewed provenance.

Refs #1429 and #1905.
cobmojo pushed a commit that referenced this pull request Sep 28, 2026
Carry the reviewed relation-ID repair preserved from #1862 in the #1905 integration candidate. Exclude only UUID-validated reference metadata from text scanning while retaining sensitive-value, visibility, tenant, field and run checks. Add deterministic valid-ID and forbidden-content regressions.

Refs #1862, #1905 and #1915.
@cobmojo cobmojo changed the title chore(skills): signed lockfile catalog refresh (128 upstreams) fix(skills): preserve Core contracts across catalog refreshes Sep 28, 2026

@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: da1f6344fd

ℹ️ 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/lib/skill-scanner-annotations.mjs
Comment thread scripts/sync-agent-skills.mjs
Comment thread scripts/refresh-upstream-skills.mjs Outdated
@greptile-apps

This comment has been minimized.

Keep uncertain multiline examples intact and scanner-visible. Reject unexpected canonical entries and propagate ecosystem publication failures while preserving complete recovery copies. Cover both CLIs with execution, parsed-payload and filesystem failure regressions.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

# one-liner and literal `git checkout .` / `git restore .` only. Restore this
# overlay after refreshing mattpocock/skills git-guardrails-claude-code.

INPUT=$(cat)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Make the hook fail fast

The new overlay reads its input without set -euo pipefail and never enables those settings, violating the scripts workflow directive. If the input read fails, the hook can continue without inspecting the command and exit successfully. Add the settings before the read. This repository requirement must be satisfied before merging.

Suggested change
INPUT=$(cat)
set -euo pipefail
INPUT=$(cat)

Context Used: scripts/AGENTS.md (source)

This branch has not been deployed

No deployments
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.

3 participants