diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 3860048db..4967ae776 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -51,9 +51,10 @@ reviews: - Distinct concurrent SKUs (different I/O or capabilities) must each be their own top-level entry — never a nested `versions` entry. - Check for potential duplicates: - - Flag if product name is very similar to an existing entry - - Flag if url matches another entry in the same collection + Check for potential duplicates (an exact name or url collision within + a collection is mechanised: `pnpm validate` warns W134/W135 and the + monthly dataset audit lists the groups, so flag only what a + normalised exact match cannot see): - Flag if the same product might exist under a different slug/name - Software products should NOT be in manufacturers/ (they belong in software/) diff --git a/.github/workflows/dataset-audit.yml b/.github/workflows/dataset-audit.yml index 35fbaf37c..c3203dcb5 100644 --- a/.github/workflows/dataset-audit.yml +++ b/.github/workflows/dataset-audit.yml @@ -34,10 +34,33 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile + # Every audit appends its settled findings to findings/findings.jsonl + # (see "Findings" in CLAUDE.md); the racks repo's file-findings.ts + # carries that file into the catalog-submissions inbox, one issue per + # finding, ever. The terminal reports are unchanged. Each audit is + # `|| true`: a red audit must not stop the next one or the issue + # update below, and the findings file is uploaded whatever happened. - name: Generate dataset audit id: audit run: | - pnpm dataset:audit --json > audit.json + mkdir -p findings + pnpm dataset:audit --json --findings findings > audit.json + + - name: Run the corpus audits with findings + run: | + pnpm power-input-audit --findings findings || true + pnpm speaker-level-audit --findings findings || true + pnpm capability-gaps --findings findings || true + pnpm prose-layout-audit --findings findings || true + wc -l findings/findings.jsonl || true + + - name: Upload findings + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: audit-findings + path: findings/findings.jsonl + if-no-files-found: ignore + retention-days: 45 - name: Create or update dataset audit issue uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 @@ -59,10 +82,12 @@ jobs: const title = '🔎 Dataset Audit'; const byCheck = audit.summary.byCheck; - // Actionable = needs a data fix. Orphan manufacturers and thin - // descriptions are informational and large; they don't gate the - // issue open/closed (otherwise it would never close). - const actionable = byCheck.duplicateName + byCheck.brokenCompatible; + // Actionable = needs a data fix. Orphan manufacturers, thin + // descriptions and stale prices are informational and large; + // they don't gate the issue open/closed (otherwise it would + // never close). Broken compatibleWith used to be counted here + // and was always zero, since E206 fails validation on it. + const actionable = byCheck.duplicateName + byCheck.duplicateUrl; const { data: issues } = await github.rest.issues.listForRepo({ owner: context.repo.owner, @@ -94,7 +119,7 @@ jobs: } const dupes = audit.findings.filter(f => f.check === 'duplicate-name'); - const broken = audit.findings.filter(f => f.check === 'broken-compatible'); + const dupeUrls = audit.findings.filter(f => f.check === 'duplicate-url'); const coverageRows = Object.entries(audit.coverage).map(([name, c]) => { const p = (n) => c.total ? `${Math.round((n / c.total) * 100)}%` : '—'; @@ -113,9 +138,10 @@ jobs: | Check | Count | Action | |-------|-------|--------| | Duplicate-name groups | ${byCheck.duplicateName} | Merge, fold into \`variants\`, or disambiguate | - | Broken \`compatibleWith\` | ${byCheck.brokenCompatible} | Fix the slug or remove the reference | + | Duplicate-url groups | ${byCheck.duplicateUrl} | Merge, or give each entry its own product page | | Orphan manufacturers | ${byCheck.orphanManufacturer} | Informational — manufacturers with no products | | Thin descriptions | ${byCheck.thinDescription} | Informational — descriptions under 40 chars | + | Makers with stale prices | ${byCheck.priceStaleness} | Informational — prices with no \`asOf\` or older than 18 months | | Flagged for LLM review | ${audit.summary.flagged} | \`/dataset-review\` | ### Coverage @@ -144,16 +170,18 @@ jobs: `); } - if (broken.length > 0) { - body += dedent(`### Broken \`compatibleWith\` references (${broken.length}) + if (dupeUrls.length > 0) { + body += dedent(`### Duplicate-url groups (${dupeUrls.length}) + + Same collection + normalized \`url\`. Usually one product filed twice; otherwise each entry needs its own product page.
Click to expand - | File | Detail | - |------|------| - ${broken.slice(0, 100).map(f => `| \`${cell(f.files[0])}\` | ${cell(f.detail)} |`).join('\n')} - ${broken.length > 100 ? `\n*... and ${broken.length - 100} more*` : ''} + | Collection | Files | Detail | + |------|------|------| + ${dupeUrls.slice(0, 100).map(f => `| ${cell(f.collection)} | ${f.files.map(x => `\`${cell(x)}\``).join(' + ')} | ${cell(f.detail)} |`).join('\n')} + ${dupeUrls.length > 100 ? `\n*... and ${dupeUrls.length - 100} more*` : ''}
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 590de0754..f1c91bded 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,15 +10,34 @@ concurrency: ${{ github.workflow }}-${{ github.ref }} permissions: contents: read +# Three jobs, split by what each holds, on the reasoning of assign-ids.yml +# and url-health.yml: a job that runs code from the tree (`pnpm install` +# executes dependency lifecycle scripts, and the build is scripts/*.ts) +# references no credential it does not need, and the job holding the +# signing key runs nothing from the tree at all. +# +# version changesets/action. It needs the checkout and +# `pnpm run version`, so it installs, and it needs a +# write token to open the version PR. That token is the +# one credential here, and it was already in the job +# that ran install before the split. +# build install, validate, build, index, checksums, SBOM. A +# read-only token and nothing else; hands dist/ on as +# an artifact. +# sign-and-release downloads that artifact, installs nothing, verifies +# minisign by digest, signs under umask 077 with the +# key in a mktemp file, attests and publishes. Until +# the split, `pnpm install` and the build ran in the +# job that wrote the signing key to /tmp under the +# default umask. jobs: - release: + version: runs-on: ubuntu-latest permissions: contents: write pull-requests: write - # For build-provenance attestation of release artifacts - id-token: write - attestations: write + outputs: + has-changesets: ${{ steps.changesets.outputs['has-changesets'] }} steps: - name: Checkout @@ -37,9 +56,6 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile - - name: Validate before release - run: pnpm validate - - name: Create Release Pull Request or Tag id: changesets uses: changesets/action@198f833dd7d863100ea6e28967bc9a9fdefadb0a # v2.1.0 @@ -49,37 +65,126 @@ jobs: commit-message: "chore: version packages" pr-title: "chore: version packages" + build: + needs: version + if: needs.version.outputs.has-changesets == 'false' + runs-on: ubuntu-latest + permissions: + contents: read + + steps: + # fetch-depth 1 is enough: the build stamps `updated_at` with the + # HEAD commit's date, which a shallow clone carries. + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + + - name: Setup Node.js + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7 + with: + node-version: "24" + + - name: Setup pnpm + uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6 + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Validate before release + run: pnpm validate + - name: Build database - if: steps.changesets.outputs['has-changesets'] == 'false' run: pnpm build - name: Build web index - if: steps.changesets.outputs['has-changesets'] == 'false' run: pnpm build:index - name: Generate checksum - if: steps.changesets.outputs['has-changesets'] == 'false' run: | sha256sum dist/catalog.sqlite > dist/catalog.sqlite.sha256 sha256sum dist/catalog-index.json > dist/catalog-index.json.sha256 - # Split from the signing step below so the signing secrets are not in - # the environment while unverified bytes are being downloaded and - # extracted. `curl | tar xz` ran an executable straight out of a - # release URL: anything that could replace that archive — upstream - # account compromise, a re-tagged release, an interfering proxy — - # chose the code that then ran alongside the catalog signing key. + # Here rather than beside the signature because the SBOM describes + # the dependencies the build ran with, and node_modules exists only + # in this job. + - name: Generate SBOM + uses: anchore/sbom-action@e22c389904149dbc22b58101806040fa8d37a610 # v0 + with: + artifact-name: catalog-sbom.spdx.json + output-file: dist/catalog-sbom.spdx.json + + - name: Upload build output + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: catalog-dist + path: | + dist/catalog.sqlite + dist/catalog.sqlite.sha256 + dist/catalog-index.json + dist/catalog-index.json.sha256 + dist/catalog-sbom.spdx.json + retention-days: 1 + if-no-files-found: error + + sign-and-release: + needs: build + runs-on: ubuntu-latest + permissions: + contents: write + # For build-provenance attestation of release artifacts + id-token: write + attestations: write + + steps: + # No `pnpm install` runs here, so no husky hooksPath gets configured + # either. The checkout is data: package.json for the version, the + # CHANGELOG for the release body, the tags for the exists check. + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Download build output + uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 + with: + name: catalog-dist + path: dist + + # The artifact was produced by a job that ran code from the tree, so + # it is data to check, not a directory to trust: exactly the files + # the build is expected to hand over, each matching its checksum. + - name: Verify build output + run: | + set -euo pipefail + expected='catalog-index.json + catalog-index.json.sha256 + catalog-sbom.spdx.json + catalog.sqlite + catalog.sqlite.sha256' + if [ "$(ls -A dist | LC_ALL=C sort)" != "$expected" ]; then + echo "::error::dist/ holds something other than the five build files; refusing to sign it" + ls -la dist + exit 1 + fi + (cd dist && sha256sum -c catalog.sqlite.sha256 catalog-index.json.sha256) + + # Verified by a digest pinned here, in review, in git, before the + # signing key exists anywhere in this job. `curl | tar xz` ran an + # executable straight out of a release URL: anything that could + # replace that archive — upstream account compromise, a re-tagged + # release, an interfering proxy — chose the code that then ran + # alongside the catalog signing key. # - # The digest is the trust anchor: it lives here, in review, in git. - # It was taken from the archive upstream signs with the minisign - # public key published in its README + # The digest was taken from the archive upstream signs with the + # minisign public key published in its README # (RWQf6LRCGA9i53mlYecO4IzT51TGPpvWucNSCh1CBM0QTaLn73Y7GFO3), whose # detached signature is minisign-0.11-linux.tar.gz.minisig on the # same release. Re-derive it the same way when bumping the version; # verifying that signature here instead would mean checking the # download with the binary out of the download. - name: Install minisign - if: steps.changesets.outputs['has-changesets'] == 'false' env: MINISIGN_VERSION: "0.11" MINISIGN_SHA256: f0a0954413df8531befed169e447a66da6868d79052ed7e892e50a4291af7ae0 @@ -94,37 +199,34 @@ jobs: rm -f "$archive" minisign -v + # The key lives in a mktemp file created under umask 077, so it is + # readable by this user alone for the seconds it exists, and the + # trap removes it however the step ends. - name: Sign catalog with minisign - if: steps.changesets.outputs['has-changesets'] == 'false' env: CATALOG_SIGNING_KEY: ${{ secrets.CATALOG_SIGNING_KEY }} CATALOG_SIGNING_KEY_PASSWORD: ${{ secrets.CATALOG_SIGNING_KEY_PASSWORD }} run: | - echo "$CATALOG_SIGNING_KEY" > /tmp/minisign.key + set -euo pipefail + umask 077 + key=$(mktemp) + trap 'rm -f "$key"' EXIT + printf '%s\n' "$CATALOG_SIGNING_KEY" > "$key" VERSION=$(node -p "require('./package.json').version") echo "$CATALOG_SIGNING_KEY_PASSWORD" | minisign -S \ - -s /tmp/minisign.key \ + -s "$key" \ -m dist/catalog.sqlite \ -t "catalog v${VERSION}" echo "$CATALOG_SIGNING_KEY_PASSWORD" | minisign -S \ - -s /tmp/minisign.key \ + -s "$key" \ -m dist/catalog-index.json \ -t "catalog-index v${VERSION}" - rm /tmp/minisign.key - - - name: Generate SBOM - if: steps.changesets.outputs['has-changesets'] == 'false' - uses: anchore/sbom-action@e22c389904149dbc22b58101806040fa8d37a610 # v0 - with: - artifact-name: catalog-sbom.spdx.json - output-file: dist/catalog-sbom.spdx.json # Sigstore build provenance for the release artifacts (SLSA). The # bundle is also attached to the release as catalog.intoto.jsonl so # consumers (and OpenSSF Scorecard) can verify it offline. Best-effort: # a bookkeeping step must never be able to break a release. - name: Attest build provenance - if: steps.changesets.outputs['has-changesets'] == 'false' id: attest continue-on-error: true uses: actions/attest-build-provenance@4d101475d8b20a2381f78447822ac1eab6504dd8 # v4 @@ -134,18 +236,16 @@ jobs: dist/catalog-index.json - name: Stage provenance bundle for release - if: steps.changesets.outputs['has-changesets'] == 'false' && steps.attest.outcome == 'success' + if: steps.attest.outcome == 'success' env: BUNDLE_PATH: ${{ steps.attest.outputs.bundle-path }} run: cp "$BUNDLE_PATH" dist/catalog.intoto.jsonl - name: Get package version - if: steps.changesets.outputs['has-changesets'] == 'false' id: pkg run: echo "version=v$(node -p "require('./package.json').version")" >> $GITHUB_OUTPUT - name: Check if tag exists - if: steps.changesets.outputs['has-changesets'] == 'false' id: tag_check run: | if git rev-parse "${{ steps.pkg.outputs.version }}" >/dev/null 2>&1; then @@ -161,7 +261,7 @@ jobs: # section and the generated list follows it for maintainers. The # version reaches awk through `env:`, never interpolated into `run:`. - name: Extract release notes from CHANGELOG - if: steps.changesets.outputs['has-changesets'] == 'false' && steps.tag_check.outputs.exists == 'false' + if: steps.tag_check.outputs.exists == 'false' env: VERSION: ${{ steps.pkg.outputs.version }} run: | @@ -174,7 +274,7 @@ jobs: fi - name: Create GitHub Release - if: steps.changesets.outputs['has-changesets'] == 'false' && steps.tag_check.outputs.exists == 'false' + if: steps.tag_check.outputs.exists == 'false' uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3 with: tag_name: ${{ steps.pkg.outputs.version }} diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index a70b6a9e2..1e8017179 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -14,8 +14,12 @@ on: - "pnpm-lock.yaml" - "pnpm-workspace.yaml" - ".github/workflows/validate.yml" - # Keep these paths in sync with the push trigger above. + # Keep these paths in sync with the push trigger above. `labeled` and + # `unlabeled` are listed because the ID immutability step reads the + # `retirement` label: without them a maintainer adding the label gets + # no new run, and a re-run keeps the original payload. pull_request: + types: [opened, synchronize, reopened, labeled, unlabeled] branches: [main] paths: - "data/**" @@ -80,8 +84,15 @@ jobs: - name: Check formatting run: pnpm format:check + # An id may leave the catalog only on purpose: the check fails a + # deletion or a rename that drops an id unless the pull request + # carries the `retirement` label. The labels reach the script through + # env, never `${{ }}` inside `run:`, since a label name is text + # somebody typed. - name: Check ID immutability if: github.event_name == 'pull_request' + env: + PR_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} run: pnpm exec tsx scripts/check-id-immutability.ts origin/main # `pnpm assign-ids` owns `id` and `io[].key`, and the build refuses an diff --git a/CLAUDE.md b/CLAUDE.md index 838c71b89..fddd30716 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -409,14 +409,16 @@ io: **IO field validation** uses a two-tier system: -- **Strict (errors, blocks CI):** `signalFlow`, `category`, `type`, `position`, `price.currency` +- **Strict (errors, blocks CI):** `signalFlow`, `category`, `type`, `connection`, `position`, `price.currency` - `signalFlow`: input, output, bidirectional - `category`: audio, midi, digital, power - `type`: closed vocabulary in `schema/io-types.yaml` — unknown values fail with E117 + - `connection`: closed vocabulary in `schema/io-connections.yaml` — unknown values fail + with E130 (the advisory W121 until the corpus carried one unknown value) - `position`: Top, Bottom, Left, Right - `currency`: ISO 4217 codes (USD, EUR, GBP, etc.) — see `schema/currencies.yaml` -- **Advisory (warnings, non-blocking):** `connection`, `link.type` - - Known values listed in `schema/io-connections.yaml`, `schema/link-types.yaml` +- **Advisory (warnings, non-blocking):** `link.type` + - Known values listed in `schema/link-types.yaml` - Unknown values produce warnings in `pnpm validate` output - Add new values to schema files via PR when they're confirmed valid @@ -508,7 +510,7 @@ two — bulk imports get this wrong. E.g. the Eventide H90 has 14 discrete jacks **Patchbays are the exception.** A `patch-bay` entry models each row as one `io` entry with `maxConnections` set to the point count (a 48-point TRS row, not 48 -entries), so W128 skips that category. TT/Bantam patch points use +entries), so E131 skips that category. TT/Bantam patch points use `connection: tt`. ## I/O Modelling Conventions @@ -794,6 +796,24 @@ the one key reported as E199 instead, checked on the raw object before any schema runs, because product images live in R2 keyed by id. A field that is genuinely new data goes into the schema in the same PR. +## Duplicates and Ids + +**A duplicate is a group, reported once.** `pnpm validate` warns W134 when +two entries in a collection share a manufacturer and a normalised name, +and W135 when they share a `url` (read without scheme, `www.` or a +trailing slash), on the first file of each group naming the others; +`pnpm dataset:audit` lists the same groups from the one implementation +in `scripts/lib/duplicates.ts`, so the two never disagree about a pair. +Both are advisory because `main` carried 66 name groups and 149 url +groups when the check was written. Reviewers are no longer asked to spot +exact collisions by eye; only a product filed under a different name is +still theirs to find. + +An `id` is the 21-character nanoid `pnpm assign-ids` writes (E129), +unique across every collection (E207), and leaves the catalog only under +a pull request labelled `retirement`, which is what lets +`check-id-immutability` fail a deletion or a rename that drops one. + ## Identifiers `identifiers` is keyed by plugin format (`au`, `vst3`, `aax`, `clap`, diff --git a/docs/VALIDATION_ERRORS.md b/docs/VALIDATION_ERRORS.md index 0f833fec6..594d4af8f 100644 --- a/docs/VALIDATION_ERRORS.md +++ b/docs/VALIDATION_ERRORS.md @@ -526,6 +526,58 @@ over another scheme has no place in `links`; mention it in `details` instead. --- +### E129: Id Is Not a 21-Character Nanoid + +The `id` field is not the 21-character nanoid `pnpm assign-ids` writes +(letters, digits, `-` and `_`). Nothing else is meant to write the field: a +hand-typed `id: 1`, a slug pasted into it or a truncated paste used to +validate, build and ship, and Studio keys `supersedes` and product images on +the value. + +**Fix:** Remove the hand-written `id` and let CI assign one. An entry may +leave the field out entirely; `assign-ids.yml` fills it on the pull request. + +--- + +### E130: Unknown IO Connection + +The `connection` field in an IO entry is not in the known connections list. +This was the advisory W121 until the corpus carried a single unknown value +(`sc`, the duplex fibre connector optical MADI uses, now in the schema), so +an unknown value is the same hard error an unknown `type` is (E117). + +**Known connections are listed in:** `schema/io-connections.yaml` + +**Fix:** If the value is a real connector, add it to +`schema/io-connections.yaml` in the same PR, verified against the manual or a +photo. If it names a signal (`line`, `midi`), it belongs in `type`. `trs` is +not a connection: the schema models the jack size (`1/4-inch`, `1/8-inch`) +and a stereo jack is one entry. + +--- + +### E131: IO Entry Collapses Several Physical Jacks + +An IO entry sets `maxConnections` greater than 1 on a single-jack connection +(`1/4-inch`, `1/8-inch`, `2.5mm`, `xlr`, `mini-xlr`, `rca`, `5-pin din`, +`7-pin din`). Those connectors carry one physical link each, so a value above +1 is several jacks collapsed into one entry, the mistake bulk imports made +most. This was the advisory W128 until the corpus carried no hit. + +**Fix:** Model each physical jack as its own `io` entry with +`maxConnections: 1` (the Eventide H90 is the worked example: fourteen +discrete jacks). If the entry really is an aggregate on a connector that +carries several links (a D-sub snake, ADAT), use that `connection` instead. + +Not flagged: names describing an intentional aggregate ("All Slots", "Bank +A"), and entries whose `primaryCategory` is `patch-bay`, where a 48-point row +is one `io` entry by design. + +**Triage tip:** `pnpm io-quality` lists the wider, lower-precision worklist +(collapsed-pair names, missing I/O, layout coverage gaps). + +--- + ### E199: Validation Error A generic validation error that doesn't fall into a more specific category. @@ -663,6 +715,19 @@ reference. --- +### E207: Id Claimed by Two Entries + +The same `id` appears on two entries, in one collection or across two. +`pnpm assign-ids` draws every collection's ids from one pool, and product +images are served from R2 keyed by id alone, so an id shared by a hardware +entry and a manufacturer is a collision even though the two tables never +meet. The check used to be per collection. + +**Fix:** Keep the id on the entry that had it first (check `git log`) and +remove it from the other; CI assigns that one a fresh id. + +--- + ## Content Errors (E3xx) ### E300: Invalid Markdown @@ -732,6 +797,19 @@ description: Use the `foo` command to run it --- +### E304: Truncated Content + +The `description` ends with an ellipsis (`...`), the way a scraped page +excerpt is cut: "featuring 12 algorithms, a looper and..." is not a +description, and nothing downstream can finish it. A trailing "and more...", +"etc...", "others..." or "so on..." is a phrase rather than a cut and is not +flagged. + +**Fix:** Replace the field with the complete text from the maker's page, or +end it at the last full sentence. + +--- + ### E305: Raw HTML in Prose A prose field (`description`, `details`, `specs`, or the same under @@ -853,37 +931,59 @@ pnpm format Warnings are non-blocking — they appear in `pnpm validate` output but do not affect the exit code or CI status. They flag values that are not in the known schema but may be valid. -### W120: Unknown IO Type +Retired codes: W120 (unknown io `type`) became E117, W121 (unknown io +`connection`) became E130, W123 (unknown `compatibleWith`) became E206, W128 +(collapsed jacks) became E131 and W132 (unknown key) became E121. A code is +never reused. -The `type` field in an IO entry is not in the known types list. +### W124: Duplicate URL -**Known types are listed in:** `schema/io-types.yaml` +A `links[].url` repeats the entry's own `url` or an earlier link in the same +entry. Studio renders every link, so the reader sees the same page twice. -**Fix:** If the value is valid, add it to `schema/io-types.yaml`. If it's a mistake, correct it in the data file. Note: `type` describes the signal characteristic (e.g., `line`, `instrument`, `headphone`), not the physical connector. +**Fix:** Remove the duplicate link entry. If the two differ only in a fragment +or a tracking parameter, keep the plain one. --- -### W121: Unknown IO Connection +### W125: Link Matches Manufacturer Homepage -The `connection` field in an IO entry is not in the known connections list. +The entry's `url`, or one of its `links[].url`, is the manufacturer's homepage +(compared without scheme, `www.` and trailing slashes). The manufacturer file +already carries that page, and Studio shows the maker beside every product, so +the link adds nothing and a product `url` that is a homepage is not a product +page. -**Known connections are listed in:** `schema/io-connections.yaml` - -**Fix:** If the value is valid, add it to `schema/io-connections.yaml`. If it's a mistake, correct it in the data file. Note: `connection` describes the physical connector (e.g., `1/4-inch`, `xlr`, `usb-c`), not the signal type. +**Fix:** Point `url` at the product's own page, or remove it so the field +reads as unknown. Drop the redundant link. --- -### W128: IO Entry May Combine Multiple Physical Jacks +### W126: Specs Line Overlaps With Structured Field -An IO entry sets `maxConnections` greater than 1 on a single-jack connection type (e.g., `1/4-inch`, `xlr`, `rca`, `5-pin din`). These connectors carry one physical link each, so a value above 1 usually means several jacks were collapsed into one entry during import. +A `specs` bullet only restates what `formats` or `platforms` already declare +("- VST3, AU and AAX", "- macOS and Windows"). Studio renders both, so the +reader sees the same fact twice, and a bullet that drifts from the structured +field misleads where the field is what search filters on. -**Fix:** Model each physical jack as its own `io` entry with `maxConnections: 1` (see the Eventide H90 for an example). If the entry really is an intentional aggregate for a connector that carries multiple links (e.g., a D-sub snake), use the correct `connection` type instead. Names containing words like "all", "slots", or "bank" are treated as aggregates and not flagged. +**Fix:** Delete the bullet; keep the structured field as the single source. +A bullet that says more than the field (a version requirement, a host it is +tested in) stays. -Not flagged: entries whose `primaryCategory` is `patch-bay`. A patchbay row is a -run of identical points that users patch into as one group, so a 48-point row -stays a single `io` entry rather than 48 near-identical ones. +--- -**Triage tip:** Run `pnpm io-quality` to see the full I/O data-quality worklist (combine candidates, collapsed-pair names, missing I/O, and column/row coverage gaps). +### W127: Entry Would Benefit From searchTerms + +The name is a short all-caps acronym (`MPC`, `SM7B`, `OB-Xa`) and the entry +carries no `searchTerms`. Search composes manufacturer and name, so a reader +typing the expansion or a spaced form finds nothing. + +Not flagged: ordinary words that happen to be capitalised (`BOOM`, `HALO`), +listed in `scripts/lib/acronym-exclusions.ts`. + +**Fix:** Add `searchTerms` with the expansion and the forms people type +(`SM-7B`, `SM 7B`). Do not add the product's own name or the manufacturer's +display name; both are already indexed. --- @@ -945,6 +1045,50 @@ the `prices` block: absent means "unknown", which is honest, where zero asserts --- +### W134: Name Shared With Another Entry of the Same Maker + +Two or more entries in one collection share a manufacturer and a name, compared +after lowercasing and dropping everything but letters and digits (`Pro-Q 3` and +`ProQ3`; a trailing `+` survives as `plus`, so `Prime 4+` is not `Prime 4`). That +is the same product filed twice, a cosmetic variant that belongs in one entry's +`variants`, or two distinct products whose names need disambiguating. Reported +once per group, on the first file, naming the others. + +Reviewers used to be asked to spot this by eye on every import PR. It is +advisory while `main` carries 66 groups; the monthly dataset audit lists the +same groups for review. + +**Fix:** Merge the entries (keep the older id, see E207), fold a colour or +finish into `variants`, or give distinct products distinct names. + +--- + +### W135: URL Shared With Another Entry in the Collection + +Two or more entries in one collection carry the same `url`, compared without +scheme, `www.` and trailing slashes. A product page belongs to one product, so +this is usually one product filed twice, and occasionally a vendor whose entries +all point at the storefront, which is a `url` to fix rather than entries to +merge. Reported once per group. Advisory while `main` carries 149 groups. + +**Fix:** Merge duplicates, or point each entry at its own product page. A +product with no page of its own leaves `url` out. + +--- + +### W136: Hardware or Accessory Entry Has No Description + +A hardware or accessory entry carries no `description`, or an empty one. +CLAUDE.md has always required the field on both collections ("Hardware +requires: name, manufacturer, primaryCategory, description"), and the schema +never did; 33 entries on `main` carry none, so the check is staged as a warning +rather than E100 until those are filled. + +**Fix:** Write one or two sentences from the maker's page in flow scalar +format. Do not paste the maker's tagline as the description. + +--- + ## Getting Help If you encounter an error not listed here, or need clarification: diff --git a/schema/CONTEXT.md b/schema/CONTEXT.md index 22cf49fbf..5c9eb0037 100644 --- a/schema/CONTEXT.md +++ b/schema/CONTEXT.md @@ -836,7 +836,7 @@ rear-panel diagram — assign them with `pnpm enrich-io `, not during bulk - `ground` - `proprietary` -### IO Connections (advisory — unknown values produce warnings) +### IO Connections (strict — unknown values fail validation with E130) - `1/4-inch` - `tt` @@ -870,6 +870,7 @@ rear-panel diagram — assign them with `pnpm enrich-io `, not during bulk - `lightning` - `aes/ebu` - `digilink` +- `sc` - `2-pin` - `db9` - `db15` diff --git a/schema/io-connections.yaml b/schema/io-connections.yaml index dfb8fbb4d..af420c04e 100644 --- a/schema/io-connections.yaml +++ b/schema/io-connections.yaml @@ -1,8 +1,9 @@ -# Valid IO connection/connector types (advisory - warning if not matched) +# Valid IO connection/connector types (closed vocabulary: E130 if not matched) # # The `connection` field describes the physical connector of an IO port. -# Add new values here as they appear in data. Unknown values generate warnings -# in `pnpm validate` and are surfaced during data review. +# An unknown value fails `pnpm validate`, the same as an unknown `type` +# (E117), so a genuinely new connector lands here in the same PR as the +# entry that needs it, verified against the manual or a photo. connections: # Quarter-inch jacks - 1/4-inch @@ -53,6 +54,7 @@ connections: - lightning - aes/ebu - digilink # Avid DigiLink (Pro Tools HD/HDX interface ports) + - sc # SC duplex fibre (optical MADI, e.g. sonible ml:mio) # Multi-pin connectors - 2-pin diff --git a/schema/json/hardware.json b/schema/json/hardware.json index db569c2ba..c325aa0d1 100644 --- a/schema/json/hardware.json +++ b/schema/json/hardware.json @@ -2481,7 +2481,7 @@ }, "connection": { "type": "string", - "description": "Physical connector (e.g., 1/4-inch, xlr, usb-c). Describes the plug/jack, not the signal type. Known values are in schema/io-connections.yaml — unknown values produce warnings." + "description": "Physical connector (e.g., 1/4-inch, xlr, usb-c). Describes the plug/jack, not the signal type. Known values are in schema/io-connections.yaml — an unknown value fails validation (E130)." }, "connectorDetail": { "type": "array", diff --git a/scripts/__tests__/check-id-immutability.test.ts b/scripts/__tests__/check-id-immutability.test.ts new file mode 100644 index 000000000..179950e33 --- /dev/null +++ b/scripts/__tests__/check-id-immutability.test.ts @@ -0,0 +1,134 @@ +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { findIdImmutabilityErrors, RETIREMENT_LABEL } from "../check-id-immutability.js"; + +// The check is run against a throwaway git repository rather than a +// mocked diff, because what it escaped on (a rename git pairs with -M, a +// deletion `--diff-filter=M` never lists) is git's own behaviour. + +const ENTRY_A = `id: AAAAAAAAAAAAAAAAAAAAA +name: Alpha +manufacturer: acme +primaryCategory: pedal +description: A pedal with enough prose for git to pair a rename. +io: + - key: K1aaaaaa + name: Input + signalFlow: input + category: audio + type: instrument + connection: 1/4-inch + maxConnections: 1 + position: Right +`; + +const ENTRY_B = ENTRY_A.replace("AAAAAAAAAAAAAAAAAAAAA", "BBBBBBBBBBBBBBBBBBBBB") + .replace("Alpha", "Beta") + .replace("K1aaaaaa", "K2bbbbbb"); + +describe("findIdImmutabilityErrors", () => { + const dirs: string[] = []; + afterEach(() => { + for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); + }); + + const git = (cwd: string, ...args: string[]) => + execFileSync("git", args, { cwd, encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"] }); + + /** A repository with two hardware entries committed; returns its path and the base sha. */ + function repo(): { cwd: string; base: string } { + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), "catalog-ids-")); + dirs.push(cwd); + git(cwd, "init", "-q"); + git(cwd, "config", "user.email", "t@example.com"); + git(cwd, "config", "user.name", "t"); + git(cwd, "config", "commit.gpgsign", "false"); + fs.mkdirSync(path.join(cwd, "data", "hardware"), { recursive: true }); + write(cwd, "a.yaml", ENTRY_A); + write(cwd, "b.yaml", ENTRY_B); + commit(cwd, "base"); + return { cwd, base: git(cwd, "rev-parse", "HEAD").trim() }; + } + const write = (cwd: string, name: string, text: string) => + fs.writeFileSync(path.join(cwd, "data", "hardware", name), text); + const commit = (cwd: string, message: string) => { + git(cwd, "add", "-A"); + git(cwd, "-c", "core.hooksPath=/dev/null", "commit", "-q", "-m", message); + }; + + it("passes an unchanged tree and a modified file that keeps its id", () => { + const { cwd, base } = repo(); + expect(findIdImmutabilityErrors({ baseRef: base, cwd })).toEqual({ errors: [], checked: 0 }); + write(cwd, "a.yaml", ENTRY_A.replace("name: Alpha", "name: Alpha II")); + commit(cwd, "rename product"); + expect(findIdImmutabilityErrors({ baseRef: base, cwd })).toEqual({ errors: [], checked: 1 }); + }); + + it("fails a modified file whose id changed", () => { + const { cwd, base } = repo(); + write(cwd, "a.yaml", ENTRY_A.replace("AAAAAAAAAAAAAAAAAAAAA", "CCCCCCCCCCCCCCCCCCCCC")); + commit(cwd, "new id"); + const { errors } = findIdImmutabilityErrors({ baseRef: base, cwd }); + expect(errors).toEqual([ + "data/hardware/a.yaml: ID was changed from 'AAAAAAAAAAAAAAAAAAAAA' to 'CCCCCCCCCCCCCCCCCCCCC'. IDs are immutable once assigned.", + ]); + }); + + it("passes a rename that keeps the id and fails one that changes it, once", () => { + const { cwd, base } = repo(); + git(cwd, "mv", "data/hardware/b.yaml", "data/hardware/b-mk2.yaml"); + commit(cwd, "rename file"); + expect(findIdImmutabilityErrors({ baseRef: base, cwd }).errors).toEqual([]); + + write(cwd, "b-mk2.yaml", ENTRY_B.replace("BBBBBBBBBBBBBBBBBBBBB", "DDDDDDDDDDDDDDDDDDDDD")); + commit(cwd, "rename with new id"); + const { errors } = findIdImmutabilityErrors({ baseRef: base, cwd }); + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch(/^data\/hardware\/b-mk2\.yaml: ID was changed from 'BBBB/); + }); + + it("fails a deletion unless the pull request carries the retirement label", () => { + const { cwd, base } = repo(); + fs.rmSync(path.join(cwd, "data", "hardware", "b.yaml")); + commit(cwd, "delete"); + const { errors } = findIdImmutabilityErrors({ baseRef: base, cwd }); + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch( + /^data\/hardware\/b\.yaml: id 'BBBBBBBBBBBBBBBBBBBBB' is on [0-9a-f]+ and on no file here\./ + ); + expect( + findIdImmutabilityErrors({ baseRef: base, cwd, labels: ["data", RETIREMENT_LABEL] }).errors + ).toEqual([]); + }); + + it("passes a deletion whose id moved to another file in the same change", () => { + // Two entries merged into one keep the surviving id; the other file goes. + const { cwd, base } = repo(); + fs.rmSync(path.join(cwd, "data", "hardware", "b.yaml")); + write(cwd, "beta.yaml", ENTRY_B); + commit(cwd, "merge"); + expect(findIdImmutabilityErrors({ baseRef: base, cwd }).errors).toEqual([]); + }); + + it("reports a port whose key moved", () => { + const { cwd, base } = repo(); + write(cwd, "a.yaml", ENTRY_A.replace("K1aaaaaa", "K9zzzzzz")); + commit(cwd, "churn key"); + const { errors } = findIdImmutabilityErrors({ baseRef: base, cwd }); + expect(errors).toEqual([ + "data/hardware/a.yaml: io port 'Input' changed key from 'K1aaaaaa' to 'K9zzzzzz'. IO keys are immutable once assigned.", + ]); + }); + + it("refuses a head file that is a symlink rather than reading through it", () => { + const { cwd, base } = repo(); + fs.rmSync(path.join(cwd, "data", "hardware", "a.yaml")); + fs.symlinkSync("/dev/null", path.join(cwd, "data", "hardware", "a.yaml")); + commit(cwd, "symlink"); + const { errors } = findIdImmutabilityErrors({ baseRef: base, cwd }); + expect(errors.some((e) => /a\.yaml: .*symbolic link/.test(e))).toBe(true); + }); +}); diff --git a/scripts/__tests__/dataset-audit.test.ts b/scripts/__tests__/dataset-audit.test.ts index 3e7c48b4e..9304c18ec 100644 --- a/scripts/__tests__/dataset-audit.test.ts +++ b/scripts/__tests__/dataset-audit.test.ts @@ -4,11 +4,12 @@ import { checkCvGateCategory, checkExpressionTypedCv, checkModularMissingHp, + checkPriceStaleness, computeHintCoverage, computeHpCoverage, type LoadedProduct, - normalizeName, } from "../dataset-audit.js"; +import { normalizeName } from "../lib/duplicates.js"; import type { Hardware } from "../lib/types.js"; import { DATA_DIR } from "../lib/utils.js"; @@ -179,3 +180,60 @@ describe("computeHpCoverage", () => { ).toEqual({ modular: 2, withHp: 1 }); }); }); + +describe("checkPriceStaleness", () => { + const now = new Date("2026-10-16T00:00:00Z"); + const priced = ( + slug: string, + maker: string, + prices: Record[] + ): LoadedProduct => ({ + type: "software", + slug, + file: path.join(DATA_DIR, "software", `${slug}.yaml`), + entry: { name: slug, manufacturer: maker, prices } as unknown as Hardware, + }); + + it("groups missing and stale asOf by manufacturer and leaves fresh prices alone", () => { + const findings = checkPriceStaleness( + [ + priced("a", "acme", [{ amount: 1, currency: "USD" }]), + priced("b", "acme", [{ amount: 1, currency: "USD", asOf: "2024-01-01" }]), + priced("c", "acme", [{ amount: 1, currency: "USD", asOf: "2026-09-01" }]), + priced("d", "other", [{ amount: 1, currency: "USD", asOf: "2025-04-16" }]), + priced("e", "other", [{ amount: 1, currency: "USD", asOf: "2025-04-15" }]), + ], + now + ); + expect(findings.map((f) => [f.name, f.files, f.detail])).toEqual([ + [ + "acme", + ["software/a.yaml", "software/b.yaml"], + "2 entries carry prices that need a re-check (1 with no asOf, 1 checked over 18 months ago).", + ], + [ + "other", + ["software/d.yaml", "software/e.yaml"], + "2 entries carry prices that need a re-check (2 checked over 18 months ago).", + ], + ]); + expect(findings.every((f) => f.check === "price-staleness" && !f.needsLlmReview)).toBe(true); + }); + + it("reads nested prices and caps the files it names", () => { + const many = Array.from({ length: 12 }, (_, i) => + priced(`p${i}`, "acme", [{ amount: 1, currency: "USD" }]) + ); + const nested: LoadedProduct = { + ...priced("v", "acme", []), + entry: { + name: "v", + manufacturer: "acme", + versions: [{ name: "1", prices: [{ amount: 1, currency: "USD" }] }], + } as unknown as Hardware, + }; + const [finding] = checkPriceStaleness([...many, nested], now); + expect(finding.files).toHaveLength(10); + expect(finding.detail).toMatch(/^13 entries .* the first 10 files are listed\.$/); + }); +}); diff --git a/scripts/__tests__/duplicates.test.ts b/scripts/__tests__/duplicates.test.ts new file mode 100644 index 000000000..08f7282af --- /dev/null +++ b/scripts/__tests__/duplicates.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from "vitest"; +import { + findDuplicateNameGroups, + findDuplicateUrlGroups, + normalizeName, + normalizeUrl, +} from "../lib/duplicates.js"; + +describe("normalizeUrl", () => { + it("drops scheme, www and trailing slashes and lowercases the rest", () => { + expect(normalizeUrl("HTTPS://www.Example.com/Serum/")).toBe("example.com/serum"); + expect(normalizeUrl(" http://example.com ")).toBe("example.com"); + }); +}); + +describe("findDuplicateNameGroups", () => { + it("groups by collection, manufacturer and normalized name, once per group", () => { + const groups = findDuplicateNameGroups([ + { collection: "software", file: "a.yaml", name: "Pro-Q 3", manufacturer: "fabfilter" }, + { collection: "software", file: "b.yaml", name: "ProQ3", manufacturer: "fabfilter" }, + { collection: "software", file: "c.yaml", name: "ProQ3", manufacturer: "other" }, + { collection: "hardware", file: "d.yaml", name: "ProQ3", manufacturer: "fabfilter" }, + { collection: "software", file: "e.yaml", name: "ProQ3+", manufacturer: "fabfilter" }, + ]); + expect(groups).toEqual([ + { + collection: "software", + value: "Pro-Q 3", + entries: [ + { collection: "software", file: "a.yaml", name: "Pro-Q 3", manufacturer: "fabfilter" }, + { collection: "software", file: "b.yaml", name: "ProQ3", manufacturer: "fabfilter" }, + ], + }, + ]); + }); + + it("never groups an entry without a manufacturer or a name", () => { + expect( + findDuplicateNameGroups([ + { collection: "manufacturers", file: "a.yaml", name: "Acme" }, + { collection: "manufacturers", file: "b.yaml", name: "Acme" }, + { collection: "software", file: "c.yaml", manufacturer: "acme" }, + { collection: "software", file: "d.yaml", manufacturer: "acme" }, + ]) + ).toEqual([]); + expect(normalizeName("")).toBe(""); + }); +}); + +describe("findDuplicateUrlGroups", () => { + it("groups by collection and normalized url", () => { + const groups = findDuplicateUrlGroups([ + { collection: "software", file: "a.yaml", url: "https://x.example/serum" }, + { collection: "software", file: "b.yaml", url: "http://www.x.example/serum/" }, + { collection: "hardware", file: "c.yaml", url: "https://x.example/serum" }, + { collection: "software", file: "d.yaml" }, + ]); + expect(groups.map((g) => [g.collection, g.value, g.entries.map((e) => e.file)])).toEqual([ + ["software", "https://x.example/serum", ["a.yaml", "b.yaml"]], + ]); + }); +}); diff --git a/scripts/__tests__/identifier-fallback.test.ts b/scripts/__tests__/identifier-fallback.test.ts index 032f938d6..9d18cdac0 100644 --- a/scripts/__tests__/identifier-fallback.test.ts +++ b/scripts/__tests__/identifier-fallback.test.ts @@ -104,9 +104,9 @@ describe("buildDatabase with fallback identifiers", () => { const ids = { acme: "MFR00000000000000acme", shared: "SW0000000000000shared", - app: "SW00000000000000000app", + app: "SW0000000000000000app", mixed: "SW00000000000000mixed", - collection: "SW00000000collection", + collection: "SW000000000collection", }; let tmp: string; diff --git a/scripts/__tests__/utils.test.ts b/scripts/__tests__/utils.test.ts index 48a664ae2..0dd87c5b9 100644 --- a/scripts/__tests__/utils.test.ts +++ b/scripts/__tests__/utils.test.ts @@ -6,6 +6,7 @@ import { escapeSQL, findClosestMatch, formatValidOptions, + getYamlFiles, loadYamlFile, parseErrorPath, readRegularFile, @@ -48,6 +49,27 @@ describe("readRegularFile", () => { }); }); +describe("getYamlFiles", () => { + const dirs: string[] = []; + afterEach(() => { + for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); + }); + + it("lists regular .yaml and .yml files, sorted, and nothing that is not a regular file", () => { + // A listing is a list of names: a symlink named x.yaml is not a catalog + // file, and neither is a directory named one. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "catalog-list-")); + dirs.push(dir); + fs.writeFileSync(path.join(dir, "b.yaml"), "name: B\n"); + fs.writeFileSync(path.join(dir, "a.yml"), "name: A\n"); + fs.writeFileSync(path.join(dir, "notes.md"), ""); + fs.symlinkSync("/dev/zero", path.join(dir, "link.yaml")); + fs.mkdirSync(path.join(dir, "dir.yaml")); + expect(getYamlFiles(dir)).toEqual([path.join(dir, "a.yml"), path.join(dir, "b.yaml")]); + expect(getYamlFiles(path.join(dir, "missing"))).toEqual([]); + }); +}); + describe("findClosestMatch", () => { it("returns exact match when found", () => { const options = new Set(["synth", "effect", "sampler"]); diff --git a/scripts/__tests__/validate.test.ts b/scripts/__tests__/validate.test.ts index 4c4498a8a..846d76cf2 100644 --- a/scripts/__tests__/validate.test.ts +++ b/scripts/__tests__/validate.test.ts @@ -11,7 +11,6 @@ import { detectSupersedeCycle, getErrorCodeFromZodIssue, isValidCategory, - normalizeToMarkdownString, validateFile, validateMarkdown, type WarningContext, @@ -286,17 +285,6 @@ describe("validateMarkdown", () => { }); }); -describe("normalizeToMarkdownString", () => { - it("returns a string unchanged", () => { - expect(normalizeToMarkdownString("one\ntwo")).toBe("one\ntwo"); - }); - - it("joins an array with blank lines as paragraph breaks", () => { - expect(normalizeToMarkdownString(["one", "two"])).toBe("one\n\ntwo"); - expect(normalizeToMarkdownString([])).toBe(""); - }); -}); - describe("detectSupersedeCycle", () => { const slugs = new Map([ ["a", "alpha"], @@ -399,6 +387,71 @@ describe("validateFile", () => { expect(validateFile(file, COLLECTION_SCHEMAS.hardware, MAKERS)).toBeNull(); }); + it("rejects an id that is not a 21-character nanoid as E129, on every collection", () => { + for (const bad of ["1", "serum", "aaaaaaaaaaaaaaaaaaaa", "aaaaaaaaaaaaaaaaaaaa!"]) { + const file = writeEntry("hardware", HARDWARE_OK.replace("aaaaaaaaaaaaaaaaaaaaa", bad)); + const result = validateFile(file, COLLECTION_SCHEMAS.hardware, MAKERS); + expect(result?.details, bad).toHaveLength(1); + expect(result?.details?.[0], bad).toMatchObject({ + code: ValidationErrorCode.E129_INVALID_ID_FORMAT, + path: "id", + line: 1, + }); + } + const maker = writeEntry("manufacturers", "id: nope\nname: Acme\n"); + expect( + validateFile(maker, COLLECTION_SCHEMAS.manufacturers, MAKERS)?.details?.[0] + ).toMatchObject({ code: ValidationErrorCode.E129_INVALID_ID_FORMAT, path: "id" }); + // Optional: assign-ids writes it after validate.yml's first run. + const none = writeEntry("hardware", HARDWARE_OK.replace("id: aaaaaaaaaaaaaaaaaaaaa\n", "")); + expect(validateFile(none, COLLECTION_SCHEMAS.hardware, MAKERS)).toBeNull(); + }); + + it("rejects a YAML array in a prose field as E101", () => { + // CLAUDE.md has always asked for a block scalar; the union that accepted + // an array was dropped once the corpus carried none. + const file = writeEntry("hardware", `${HARDWARE_OK}specs:\n - one\n - two\n`); + expect(validateFile(file, COLLECTION_SCHEMAS.hardware, MAKERS)?.details?.[0]).toMatchObject({ + code: ValidationErrorCode.E101_INVALID_FIELD_TYPE, + path: "specs", + }); + }); + + it("rejects an unknown io connection as E130, keyed to the connection line", () => { + // The advisory W121 until the vocabulary was clean; `trs` is the + // mistake it meets most (the schema models the jack size instead). + const file = writeEntry( + "hardware", + HARDWARE_OK.replace("connection: 1/4-inch", "connection: trs") + ); + const result = validateFile(file, COLLECTION_SCHEMAS.hardware, MAKERS); + expect(result?.details).toHaveLength(1); + expect(result?.details?.[0]).toMatchObject({ + code: ValidationErrorCode.E130_UNKNOWN_IO_CONNECTION, + path: "io.0.connection", + line: 13, + }); + }); + + it("rejects several jacks collapsed into one io entry as E131, unless the entry is a patchbay", () => { + const collapsed = HARDWARE_OK.replace("maxConnections: 1", "maxConnections: 2"); + const result = validateFile( + writeEntry("hardware", collapsed), + COLLECTION_SCHEMAS.hardware, + MAKERS + ); + expect(result?.details).toHaveLength(1); + expect(result?.details?.[0]).toMatchObject({ + code: ValidationErrorCode.E131_IO_COLLAPSED_JACKS, + path: "io.0.maxConnections", + line: 14, + }); + const patchbay = collapsed.replace("primaryCategory: pedal", "primaryCategory: patch-bay"); + expect( + validateFile(writeEntry("hardware", patchbay), COLLECTION_SCHEMAS.hardware, MAKERS) + ).toBeNull(); + }); + it("rejects a negative or non-finite price amount as E127", () => { // `.nan` and `.inf` are reachable from YAML, and z.number() refuses // both at the base type check, so they never reach the `.check()`. @@ -783,10 +836,16 @@ describe("collectWarnings", () => { expect(collectWarnings(file, data as WarningContext, document, lineCounter)).toBeNull(); }); - it("W121: unknown io connection, keyed to the connection line", () => { - expect(warn(HARDWARE_OK.replace("connection: 1/4-inch", "connection: trs"))).toEqual([ - { code: "W121", path: "io[0].connection", line: 13 }, + it("W136: a hardware or accessory entry with no description, keyed to the name line", () => { + // CLAUDE.md requires the field on both collections; the schema never + // did, and 33 entries on main carry none, so it is staged as a warning. + const bare = HARDWARE_OK.replace("description: A pedal.\n", ""); + expect(warn(bare)).toEqual([{ code: "W136", path: "description", line: 2 }]); + expect(warn(bare.replace("categories:", "description: ''\ncategories:"))).toEqual([ + { code: "W136", path: "description", line: 2 }, ]); + expect(warn(bare, {}, "accessories")).toEqual([{ code: "W136", path: "description", line: 2 }]); + expect(warn(bare, {}, "software")).toEqual([]); }); it("W133: a free price with no provenance, and none once it carries some", () => { @@ -819,14 +878,6 @@ describe("collectWarnings", () => { ).toEqual([{ code: "W133", path: "versions[0].prices[0]", line: 19 }]); }); - it("W128: several jacks collapsed into one entry, unless the entry is a patchbay", () => { - const collapsed = HARDWARE_OK.replace("maxConnections: 1", "maxConnections: 2"); - expect(warn(collapsed)).toEqual([{ code: "W128", path: "io[0].maxConnections", line: 14 }]); - expect(warn(collapsed.replace("primaryCategory: pedal", "primaryCategory: patch-bay"))).toEqual( - [] - ); - }); - it("W124: a link repeating url or an earlier link", () => { const yaml = `${HARDWARE_OK}url: https://acme.example/widget links: @@ -923,9 +974,9 @@ specs: |- it("accumulates every warning the file earns", () => { const yaml = HARDWARE_OK.replace("name: Widget", "name: ADT | Acme").replace( - "connection: 1/4-inch", - "connection: trs" + "description: A pedal.\n", + "" ); - expect(warn(yaml).map((w) => w.code)).toEqual(["W121", "W130"]); + expect(warn(yaml).map((w) => w.code)).toEqual(["W136", "W130"]); }); }); diff --git a/scripts/assign-ids.ts b/scripts/assign-ids.ts index ac12ba479..210e28970 100644 --- a/scripts/assign-ids.ts +++ b/scripts/assign-ids.ts @@ -17,19 +17,7 @@ import { nanoid } from "nanoid"; import { parseDocument } from "yaml"; import { assignIoKeys } from "./lib/io-keys.js"; import type { Collection } from "./lib/types.js"; - -const REPO_ROOT = path.resolve(import.meta.dirname, ".."); -const DATA_DIR = path.join(REPO_ROOT, "data"); - -function getYamlFiles(dir: string): string[] { - if (!fs.existsSync(dir)) { - return []; - } - return fs - .readdirSync(dir) - .filter((f) => f.endsWith(".yaml") || f.endsWith(".yml")) - .map((f) => path.join(dir, f)); -} +import { DATA_DIR, getYamlFiles, readRegularFile } from "./lib/utils.js"; /** Load all existing IDs across all collections into a Set */ function loadExistingIds(): Set { @@ -45,7 +33,7 @@ function loadExistingIds(): Set { for (const collection of collections) { const files = getYamlFiles(path.join(DATA_DIR, collection)); for (const file of files) { - const content = fs.readFileSync(file, "utf-8"); + const content = readRegularFile(file); const doc = parseDocument(content); const data = doc.toJSON() as Record; if (typeof data.id === "string" && data.id.length > 0) { @@ -82,7 +70,7 @@ function assignIds(): void { const files = getYamlFiles(path.join(DATA_DIR, collection)); for (const file of files) { - const content = fs.readFileSync(file, "utf-8"); + const content = readRegularFile(file); const doc = parseDocument(content); const data = doc.toJSON() as Record; diff --git a/scripts/build-sqlite.ts b/scripts/build-sqlite.ts index 3647deb19..26ab4a3f9 100644 --- a/scripts/build-sqlite.ts +++ b/scripts/build-sqlite.ts @@ -6,6 +6,7 @@ * Run with: pnpm build */ +import { execFileSync } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; import { pathToFileURL } from "node:url"; @@ -1566,12 +1567,32 @@ function populateDatabase(db: Database.Database, dataDir: string, version: strin VALUES (?, ?) `); updateMeta.run("version", version); - updateMeta.run("updated_at", new Date().toISOString()); + updateMeta.run("updated_at", buildTimestamp()); updateMeta.run("schema_version", CATALOG_SCHEMA_VERSION); return softwareCount + contentCount + hardwareCount + accessoryCount; } +/** + * The `updated_at` stamp: the HEAD commit's date, so two builds of one + * commit produce one database (the release attests the bytes, and a + * wall-clock stamp made every rebuild a different artifact). The clock + * is the fallback for a build outside a checkout. + */ +function buildTimestamp(): string { + try { + const committed = execFileSync("git", ["log", "-1", "--format=%cI"], { + encoding: "utf-8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); + const date = new Date(committed); + if (!Number.isNaN(date.getTime())) return date.toISOString(); + } catch { + // Not a git checkout, or git is missing: the clock is all there is. + } + return new Date().toISOString(); +} + // ============================================================================= // MAIN // ============================================================================= diff --git a/scripts/check-id-immutability.ts b/scripts/check-id-immutability.ts index 6e1bdb021..55bcb2d8c 100644 --- a/scripts/check-id-immutability.ts +++ b/scripts/check-id-immutability.ts @@ -1,72 +1,201 @@ /** * ID Immutability Check * - * Ensures that existing IDs are not modified in PRs. - * Compares modified YAML files against the base branch. + * An entry's `id` is the key Studio stores in a user's setups and the key + * product images are served under, so once assigned it never changes and + * never disappears. This compares the pull request's tree with the base + * branch and fails on three things: + * + * - a modified or renamed file whose `id` differs from the base; + * - an `id` present on the base that no file on the head carries, which + * is what a deletion or a rename-with-new-id looks like; + * - a hardware port whose stable `key` moved (see io-keys.ts). + * + * Until this read `--diff-filter=MRD` it listed modified files only and + * `continue`d when either side was missing, so a rename that changed the + * id and a plain deletion both passed. A retirement is the one legitimate + * way an id leaves the catalog, and it is declared rather than inferred: + * the pull request carries the `retirement` label, which validate.yml + * passes in `PR_LABELS`. + * * Run with: tsx scripts/check-id-immutability.ts */ import { execFileSync } from "node:child_process"; -import fs from "node:fs"; import path from "node:path"; +import { pathToFileURL } from "node:url"; import { parse as parseYaml } from "yaml"; import { findIoKeyViolations } from "./lib/io-keys.js"; import type { IO } from "./lib/types.js"; +import { readRegularFile } from "./lib/utils.js"; + +/** The label that lets an id leave the catalog on purpose. */ +export const RETIREMENT_LABEL = "retirement"; + +interface Entry { + id?: unknown; + io?: IO[]; +} + +interface DataChange { + status: "M" | "R" | "D" | "T"; + /** Path on the base branch. */ + basePath: string; + /** Path on the head, absent for a deletion. */ + headPath?: string; +} + +export interface CheckOptions { + baseRef: string; + /** The checkout to compare, the repository root. */ + cwd: string; + /** The pull request's labels, as validate.yml hands them over. */ + labels?: readonly string[]; +} -const baseRef = process.argv[2] ?? "origin/main"; +function git(cwd: string, args: string[]): string { + return execFileSync("git", args, { cwd, encoding: "utf-8" }); +} -function getModifiedDataFiles(): string[] { +/** + * The data files the head changed against the base: modified, renamed + * (with `-M` so git pairs the old and new path), deleted, and type-changed + * (a file replaced by a symlink is `T`, not `M`, and is the one case the + * guarded read below exists for). A `--name-only` listing cannot say which + * a path is, so this reads `--name-status`, where a rename row carries + * both paths. + */ +function getChangedDataFiles(cwd: string, baseRef: string): DataChange[] { + let output: string; try { - const output = execFileSync( - "git", - ["diff", "--name-only", "--diff-filter=M", baseRef, "--", "data/**/*.yaml"], - { encoding: "utf-8" } - ); - return output.split("\n").filter(Boolean); + output = git(cwd, [ + "diff", + "--name-status", + "-M", + "--diff-filter=MRDT", + baseRef, + "--", + "data/**/*.yaml", + ]); } catch { return []; } + const changes: DataChange[] = []; + for (const row of output.split("\n")) { + if (!row) continue; + const [code, first, second] = row.split("\t"); + const status = code?.[0]; + if ((status === "M" || status === "T") && first) + changes.push({ status, basePath: first, headPath: first }); + else if (status === "R" && first && second) + changes.push({ status, basePath: first, headPath: second }); + else if (status === "D" && first) changes.push({ status, basePath: first }); + } + return changes; } -function getBaseFileContent(filePath: string): string | null { +function readBaseEntry(cwd: string, baseRef: string, filePath: string): Entry | null { try { - return execFileSync("git", ["show", `${baseRef}:${filePath}`], { - encoding: "utf-8", - }); + return parseYaml(git(cwd, ["show", `${baseRef}:${filePath}`])) as Entry; } catch { return null; } } -function checkIdImmutability(): void { - const modifiedFiles = getModifiedDataFiles(); - const errors: string[] = []; - - for (const file of modifiedFiles) { - const baseContent = getBaseFileContent(file); - if (!baseContent) continue; +function readHeadEntry(cwd: string, filePath: string): Entry { + return parseYaml(readRegularFile(path.join(cwd, filePath))) as Entry; +} - const baseData = parseYaml(baseContent) as { id?: unknown; io?: IO[] }; +/** + * Every id the working tree carries, read off one `git grep` rather than + * 21k file reads. The working tree and not HEAD, because the file check + * above reads the working tree too and the two have to agree about what + * "here" is. + */ +function idsInWorkingTree(cwd: string): Set { + const ids = new Set(); + let output: string; + try { + output = git(cwd, ["grep", "-h", "-E", "^id: ", "--", "data/**/*.yaml"]); + } catch { + // git grep exits 1 when nothing matches, which an empty tree does. + return ids; + } + for (const line of output.split("\n")) { + const id = line.replace(/^id:\s*/, "").trim(); + if (id) ids.add(id.replace(/^["']|["']$/g, "")); + } + return ids; +} - const currentPath = path.resolve(file); - if (!fs.existsSync(currentPath)) continue; +/** The violations in `cwd` against `baseRef`, each a line for the log. */ +export function findIdImmutabilityErrors(options: CheckOptions): { + errors: string[]; + checked: number; +} { + const { baseRef, cwd } = options; + const changes = getChangedDataFiles(cwd, baseRef); + const errors: string[] = []; + /** Files whose id change is already reported, so a rename is not reported twice. */ + const reported = new Set(); - const currentContent = fs.readFileSync(currentPath, "utf-8"); - const currentData = parseYaml(currentContent) as { id?: unknown; io?: IO[] }; + for (const change of changes) { + if (!change.headPath) continue; + const baseData = readBaseEntry(cwd, baseRef, change.basePath); + if (!baseData) continue; + let currentData: Entry; + try { + currentData = readHeadEntry(cwd, change.headPath); + } catch (err) { + errors.push(`${change.headPath}: ${err instanceof Error ? err.message : String(err)}`); + continue; + } if (baseData.id !== undefined && currentData.id !== baseData.id) { + reported.add(change.basePath); errors.push( - `${file}: ID was changed from '${baseData.id}' to '${currentData.id}'. IDs are immutable once assigned.` + `${change.headPath}: ID was changed from '${baseData.id}' to '${currentData.id}'. IDs are immutable once assigned.` ); } if (Array.isArray(baseData.io) && Array.isArray(currentData.io)) { for (const violation of findIoKeyViolations(baseData.io, currentData.io)) { - errors.push(`${file}: ${violation}`); + errors.push(`${change.headPath}: ${violation}`); } } } + // An id on the base that the head no longer carries anywhere. Checked + // against the whole head tree rather than the changed file, because a + // retired file's id could legitimately move to another file in the same + // pull request (a merge of two entries keeps one id). + const departures = changes.filter( + (c) => (c.status === "R" || c.status === "D") && !reported.has(c.basePath) + ); + if (departures.length > 0 && !(options.labels ?? []).includes(RETIREMENT_LABEL)) { + const headIds = idsInWorkingTree(cwd); + for (const change of departures) { + const baseData = readBaseEntry(cwd, baseRef, change.basePath); + const id = baseData?.id; + if (typeof id !== "string" || headIds.has(id)) continue; + errors.push( + `${change.basePath}: id '${id}' is on ${baseRef} and on no file here. ` + + `An id never leaves the catalog by accident; label the pull request '${RETIREMENT_LABEL}' to retire it.` + ); + } + } + + return { errors, checked: changes.length }; +} + +function main(): void { + const baseRef = process.argv[2] ?? "origin/main"; + const labels = (process.env.PR_LABELS ?? "") + .split(",") + .map((label) => label.trim()) + .filter(Boolean); + const { errors, checked } = findIdImmutabilityErrors({ baseRef, cwd: process.cwd(), labels }); + if (errors.length > 0) { console.error("❌ ID immutability check failed!\n"); for (const error of errors) { @@ -75,7 +204,10 @@ function checkIdImmutability(): void { process.exit(1); } - console.log(`✅ ID immutability check passed (${modifiedFiles.length} files checked)`); + console.log(`✅ ID immutability check passed (${checked} files checked)`); } -checkIdImmutability(); +// Run only as a CLI, so the test suite can import the check. +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main(); +} diff --git a/scripts/dataset-audit.ts b/scripts/dataset-audit.ts index d48e938e6..e07e78812 100644 --- a/scripts/dataset-audit.ts +++ b/scripts/dataset-audit.ts @@ -6,9 +6,8 @@ * per-file: they check each entry in isolation. As the catalog grows past * ~12k entries — with imports dropping hundreds of products per PR — the * problems that slip through are *cross-entry*: the same product filed - * twice, a `compatibleWith` pointing at a slug that doesn't exist, a - * manufacturer with no products, descriptions that are technically present - * but useless. + * twice, a manufacturer with no products, descriptions that are + * technically present but useless, prices nobody has checked in years. * * This script loads the WHOLE dataset and runs the deterministic * cross-entry checks that scale with dataset size (cheap, no LLM). It is @@ -25,9 +24,11 @@ * only (not fuzzy) — a product catalog is full of * model-number families ("Bassysm-F" vs "Bassysm-J") * that edit-distance matching drowns you in. - * 2. broken-compatible — `compatibleWith` slug resolves to no software or - * hardware entry. (validate fails on this as E206; - * kept here so the audit report stays complete.) + * The same groups `pnpm validate` warns on (W134), + * from the one implementation in lib/duplicates.ts. + * 2. duplicate-url — same collection + normalized `url` (W135 in + * validate). Two entries on one product page are + * usually one product filed twice. * 3. orphan-manufacturer — a manufacturer referenced by zero products. * 4. thin-description — description present but suspiciously short. * 5. aggregator-url — canonical `url` points at an aggregator @@ -43,6 +44,13 @@ * 8. modular-missing-hp — a hardware entry filed under `modular` with no * `hp`, so the faceplate width campaign can see * its remaining coverage gap (AUREO-1110). + * 9. price-staleness — prices with no `asOf`, or one older than + * eighteen months, grouped by manufacturer so a + * re-check is one vendor's store at a time. + * + * `broken-compatible` used to be check 2. `compatibleWith` to an unknown + * slug has been the error E206 since the advisory W123 was promoted, so + * the count was always zero and the monthly issue gated on it anyway. * * Findings with `needsLlmReview: true` are collected into `flagged`, the * seam handed to Tier 2. Deterministic findings (broken refs, orphans) are @@ -57,8 +65,10 @@ import path from "node:path"; import { z } from "zod"; import { isAggregatorUrl, isManufacturerOwnDomain, urlHost } from "./lib/aggregator-domains.js"; +import { findDuplicateNameGroups, findDuplicateUrlGroups } from "./lib/duplicates.js"; import { type FindingInput, findingsDirArg, recordFindings } from "./lib/findings.js"; import { countHintCoverage, emptyHintCoverage, type HintCoverage } from "./lib/io-hints.js"; +import { collectPriceArrays } from "./lib/price-terms.js"; import type { Accessory, Content, Hardware, Manufacturer, Software } from "./lib/types.js"; import { DATA_DIR, getYamlFiles, loadYamlFile, REPO_ROOT } from "./lib/utils.js"; @@ -75,6 +85,10 @@ type Product = Software | Content | Hardware | Accessory; const THIN_DESCRIPTION_CHARS = 40; /** Cap on the LLM-bound `flagged` list, prioritized by severity. */ const MAX_FLAGGED = 100; +/** A price checked longer ago than this is stale, whatever the vendor. */ +const PRICE_STALE_MONTHS = 18; +/** Files named on a per-manufacturer finding, so a vendor with 300 entries stays one row. */ +const STALE_PRICE_FILES_SHOWN = 10; // ============================================================================= // SCHEMAS @@ -82,7 +96,7 @@ const MAX_FLAGGED = 100; const CheckSchema = z.enum([ "duplicate-name", - "broken-compatible", + "duplicate-url", "orphan-manufacturer", "thin-description", "aggregator-url", @@ -92,6 +106,7 @@ const CheckSchema = z.enum([ "cv-gate-category", "expression-typed-cv", "modular-missing-hp", + "price-staleness", ]); const SeveritySchema = z.enum(["blocking", "warning", "info"]); @@ -143,7 +158,7 @@ const DatasetAuditSchema = z.object({ manufacturers: z.number(), byCheck: z.object({ duplicateName: z.number(), - brokenCompatible: z.number(), + duplicateUrl: z.number(), orphanManufacturer: z.number(), thinDescription: z.number(), aggregatorUrl: z.number(), @@ -153,6 +168,7 @@ const DatasetAuditSchema = z.object({ cvGateCategory: z.number(), expressionTypedCv: z.number(), modularMissingHp: z.number(), + priceStaleness: z.number(), }), flagged: z.number(), }), @@ -198,20 +214,6 @@ function slugOf(file: string): string { return path.basename(file, path.extname(file)); } -/** Collapse a display name to a comparable key (lowercase, alphanumerics only). */ -export function normalizeName(name: string): string { - return ( - name - .toLowerCase() - // A "+" marks a distinct SKU, so preserve it as a "plus" token instead of - // stripping it. This covers both a trailing "+" (ProFX10v3 vs ProFX10v3+, - // Prime 4 vs Prime 4+) and an internal "+" that precedes more of the name - // (Vision+ Console vs Vision Console, SPS-1UW+ MKII vs SPS-1UW MKII). - .replace(/\+/g, "plus") - .replace(/[^a-z0-9]+/g, "") - ); -} - function descriptionText(entry: Product): string { return (entry.description ?? "").trim(); } @@ -220,12 +222,6 @@ function hasPrices(entry: Product): boolean { return Array.isArray(entry.prices) && entry.prices.length > 0; } -/** `compatibleWith` only exists on software and content in the schema. */ -function compatibleWithOf(entry: Product): string[] { - const value = (entry as Software | Content).compatibleWith; - return Array.isArray(value) ? value : []; -} - // ============================================================================= // LOAD // ============================================================================= @@ -233,15 +229,12 @@ function compatibleWithOf(entry: Product): string[] { interface Dataset { products: LoadedProduct[]; manufacturers: { slug: string; file: string; entry: Manufacturer }[]; - /** Slugs that a `compatibleWith` is allowed to point at (software + hardware). */ - hostSlugs: Set; /** Manufacturer slugs referenced by at least one product. */ usedManufacturers: Set; } function loadDataset(): Dataset { const products: LoadedProduct[] = []; - const hostSlugs = new Set(); const usedManufacturers = new Set(); for (const type of PRODUCT_TYPES) { @@ -249,7 +242,6 @@ function loadDataset(): Dataset { const entry = loadYamlFile(file); const slug = slugOf(file); products.push({ type, slug, file, entry }); - if (type === "software" || type === "hardware") hostSlugs.add(slug); if (entry.manufacturer) usedManufacturers.add(entry.manufacturer); } } @@ -260,7 +252,19 @@ function loadDataset(): Dataset { entry: loadYamlFile(file), })); - return { products, manufacturers, hostSlugs, usedManufacturers }; + return { products, manufacturers, usedManufacturers }; +} + +/** A product as the duplicate finder reads it: collection, path, name, maker, url. */ +function duplicateCandidate(p: LoadedProduct) { + return { + collection: p.type, + file: relPath(p.file), + name: p.entry.name, + manufacturer: p.entry.manufacturer, + url: (p.entry as { url?: string }).url, + product: p, + }; } // ============================================================================= @@ -275,23 +279,13 @@ function loadDataset(): Dataset { * "Bassysm-J", "Amethyst" vs "Amethyst2") that edit-distance matching would * flag by the thousands. An exact collision is still worth an LLM glance — * usually a duplicate or a cosmetic variant that belongs in one entry's - * `variants`, occasionally a genuine pair of distinct products. + * `variants`, occasionally a genuine pair of distinct products. The + * grouping is `lib/duplicates.ts`, shared with validate's W134. */ function checkDuplicateNames(products: LoadedProduct[]): Finding[] { - const buckets = new Map(); - for (const p of products) { - const norm = normalizeName(p.entry.name); - if (!norm) continue; - const key = `${p.type} ${p.entry.manufacturer ?? ""} ${norm}`; - const list = buckets.get(key) ?? []; - list.push(p); - buckets.set(key, list); - } - const findings: Finding[] = []; - for (const group of buckets.values()) { - if (group.length < 2) continue; - const first = group[0]; + for (const group of findDuplicateNameGroups(products.map(duplicateCandidate))) { + const first = group.entries[0].product; findings.push({ check: "duplicate-name", severity: "warning", @@ -299,9 +293,9 @@ function checkDuplicateNames(products: LoadedProduct[]): Finding[] { collection: first.type, name: first.entry.name, manufacturer: first.entry.manufacturer, - files: group.map((g) => relPath(g.file)), + files: group.entries.map((e) => e.file), detail: - `${group.length} entries share the normalized name "${normalizeName(first.entry.name)}" ` + + `${group.entries.length} entries share the name "${group.value}" ` + "under the same manufacturer — likely the same product filed more than once " + "(cosmetic variants belong in a single entry's `variants`).", }); @@ -310,24 +304,95 @@ function checkDuplicateNames(products: LoadedProduct[]): Finding[] { } /** - * `compatibleWith` slugs that resolve to no software/hardware entry. These - * are deterministic data errors — actionable without LLM judgment. + * Same collection + normalized `url`. A product page belongs to one + * product, so two entries on it are usually one product filed twice, and + * occasionally a vendor whose pages are one storefront (which is a url to + * fix, not an entry to merge). Shared with validate's W135. */ -function checkBrokenCompatible(products: LoadedProduct[], hostSlugs: Set): Finding[] { +function checkDuplicateUrls(products: LoadedProduct[]): Finding[] { const findings: Finding[] = []; - for (const p of products) { - const refs = compatibleWithOf(p.entry); - const broken = refs.filter((slug) => !hostSlugs.has(slug)); - if (broken.length === 0) continue; + for (const group of findDuplicateUrlGroups(products.map(duplicateCandidate))) { + const first = group.entries[0].product; findings.push({ - check: "broken-compatible", + check: "duplicate-url", severity: "warning", + needsLlmReview: true, + collection: first.type, + name: first.entry.name, + manufacturer: first.entry.manufacturer, + files: group.entries.map((e) => e.file), + detail: + `${group.entries.length} entries share the url ${group.value} — ` + + "likely the same product filed more than once; otherwise each needs its own product page.", + }); + } + return findings; +} + +/** Months between two dates, whole, ignoring the day. */ +function monthsBetween(from: Date, to: Date): number { + return ( + (to.getUTCFullYear() - from.getUTCFullYear()) * 12 + (to.getUTCMonth() - from.getUTCMonth()) + ); +} + +/** + * Prices nobody has checked lately: `asOf` missing, or older than + * eighteen months. One finding per manufacturer rather than per entry, + * because a re-check is a pass over one vendor's store and a list of + * 2,000 entries is not a worklist. Informational, and never a Tier-2 + * finding: the fix is to open the page, which no model can do here. + */ +export function checkPriceStaleness( + products: readonly LoadedProduct[], + now = new Date() +): Finding[] { + interface Tally { + manufacturer: string; + files: string[]; + missing: number; + stale: number; + } + const byMaker = new Map(); + for (const p of products) { + let missing = 0; + let stale = 0; + for (const { prices } of collectPriceArrays(p.entry)) { + for (const price of prices) { + const asOf = (price as { asOf?: unknown }).asOf; + const checked = + asOf instanceof Date ? asOf : typeof asOf === "string" && asOf ? new Date(asOf) : null; + if (!checked || Number.isNaN(checked.getTime())) missing++; + else if (monthsBetween(checked, now) >= PRICE_STALE_MONTHS) stale++; + } + } + if (missing === 0 && stale === 0) continue; + const maker = p.entry.manufacturer ?? "(none)"; + const tally = byMaker.get(maker) ?? { manufacturer: maker, files: [], missing: 0, stale: 0 }; + tally.files.push(relPath(p.file)); + tally.missing += missing; + tally.stale += stale; + byMaker.set(maker, tally); + } + + const findings: Finding[] = []; + for (const tally of [...byMaker.values()].sort((a, b) => b.files.length - a.files.length)) { + const parts: string[] = []; + if (tally.missing > 0) parts.push(`${tally.missing} with no asOf`); + if (tally.stale > 0) parts.push(`${tally.stale} checked over ${PRICE_STALE_MONTHS} months ago`); + findings.push({ + check: "price-staleness", + severity: "info", needsLlmReview: false, - collection: p.type, - name: p.entry.name, - manufacturer: p.entry.manufacturer, - files: [relPath(p.file)], - detail: `compatibleWith references no existing software/hardware: ${broken.join(", ")}`, + collection: "prices", + name: tally.manufacturer, + manufacturer: tally.manufacturer, + files: tally.files.slice(0, STALE_PRICE_FILES_SHOWN), + detail: + `${tally.files.length} entries carry prices that need a re-check (${parts.join(", ")})` + + (tally.files.length > STALE_PRICE_FILES_SHOWN + ? `; the first ${STALE_PRICE_FILES_SHOWN} files are listed.` + : "."), }); } return findings; @@ -737,7 +802,7 @@ function generateAudit(fast: boolean): DatasetAudit { const findings: Finding[] = [ ...checkDuplicateNames(dataset.products), - ...checkBrokenCompatible(dataset.products, dataset.hostSlugs), + ...checkDuplicateUrls(dataset.products), ...checkOrphanManufacturers(dataset), ...(fast ? [] : checkThinDescriptions(dataset.products)), ...checkAggregatorUrls(dataset), @@ -747,6 +812,7 @@ function generateAudit(fast: boolean): DatasetAudit { ...checkCvGateCategory(dataset.products), ...checkExpressionTypedCv(dataset.products), ...checkModularMissingHp(dataset.products), + ...checkPriceStaleness(dataset.products), ]; // The Tier-2 seam: only ambiguous findings, highest severity first, capped. @@ -757,7 +823,7 @@ function generateAudit(fast: boolean): DatasetAudit { const byCheck = { duplicateName: findings.filter((f) => f.check === "duplicate-name").length, - brokenCompatible: findings.filter((f) => f.check === "broken-compatible").length, + duplicateUrl: findings.filter((f) => f.check === "duplicate-url").length, orphanManufacturer: findings.filter((f) => f.check === "orphan-manufacturer").length, thinDescription: findings.filter((f) => f.check === "thin-description").length, aggregatorUrl: findings.filter((f) => f.check === "aggregator-url").length, @@ -767,6 +833,7 @@ function generateAudit(fast: boolean): DatasetAudit { cvGateCategory: findings.filter((f) => f.check === "cv-gate-category").length, expressionTypedCv: findings.filter((f) => f.check === "expression-typed-cv").length, modularMissingHp: findings.filter((f) => f.check === "modular-missing-hp").length, + priceStaleness: findings.filter((f) => f.check === "price-staleness").length, }; return { @@ -802,7 +869,7 @@ function printConsoleReport(audit: DatasetAudit): void { console.log(` Product entries: ${audit.summary.totalEntries}`); console.log(` Manufacturers: ${audit.summary.manufacturers}`); console.log(` Duplicate-name groups: ${audit.summary.byCheck.duplicateName}`); - console.log(` Broken compatibleWith: ${audit.summary.byCheck.brokenCompatible}`); + console.log(` Duplicate-url groups: ${audit.summary.byCheck.duplicateUrl}`); console.log(` Orphan manufacturers: ${audit.summary.byCheck.orphanManufacturer}`); console.log(` Thin descriptions: ${audit.summary.byCheck.thinDescription}`); console.log(` Aggregator urls: ${audit.summary.byCheck.aggregatorUrl}`); @@ -812,6 +879,7 @@ function printConsoleReport(audit: DatasetAudit): void { console.log(` CV/clock not audio: ${audit.summary.byCheck.cvGateCategory}`); console.log(` Expression typed cv: ${audit.summary.byCheck.expressionTypedCv}`); console.log(` Modular without hp: ${audit.summary.byCheck.modularMissingHp}`); + console.log(` Makers with stale prices: ${audit.summary.byCheck.priceStaleness}`); console.log(` Flagged for LLM review: ${audit.summary.flagged}`); console.log(); @@ -848,7 +916,7 @@ function printConsoleReport(audit: DatasetAudit): void { const groups: { title: string; check: Finding["check"]; limit: number }[] = [ { title: "🔁 Duplicate-name groups", check: "duplicate-name", limit: 30 }, - { title: "🔗 Broken compatibleWith references", check: "broken-compatible", limit: 30 }, + { title: "🔗 Duplicate-url groups", check: "duplicate-url", limit: 30 }, { title: "🏷 Orphan manufacturers", check: "orphan-manufacturer", limit: 20 }, { title: "📝 Thin descriptions", check: "thin-description", limit: 20 }, { title: "🔀 Aggregator canonical urls", check: "aggregator-url", limit: 20 }, @@ -858,6 +926,7 @@ function printConsoleReport(audit: DatasetAudit): void { { title: "🎛 CV and clock jacks outside audio", check: "cv-gate-category", limit: 20 }, { title: "🦶 Expression jacks typed cv/gate", check: "expression-typed-cv", limit: 20 }, { title: "📏 Modular entries without hp", check: "modular-missing-hp", limit: 20 }, + { title: "💸 Makers with stale prices", check: "price-staleness", limit: 20 }, ]; for (const { title, check, limit } of groups) { @@ -886,7 +955,7 @@ function printConsoleReport(audit: DatasetAudit): void { /** * The inbox rows for this audit's findings. * - * `modular-missing-hp` only, of the eleven checks. The rest are either + * `modular-missing-hp` only, of the twelve checks. The rest are either * whole-dataset relations that name several files at once * (duplicate-name, orphan-manufacturer) and so have no single entry to * close an issue against, or they already have a tool that settles them diff --git a/scripts/generate-context.ts b/scripts/generate-context.ts index 60424e379..d28cd5981 100644 --- a/scripts/generate-context.ts +++ b/scripts/generate-context.ts @@ -296,7 +296,7 @@ rear-panel diagram — assign them with \`pnpm enrich-io \`, not during bu ${context.ioTypes.map((v) => `- \`${v}\``).join("\n")} -### IO Connections (advisory — unknown values produce warnings) +### IO Connections (strict — unknown values fail validation with E130) ${context.ioConnections.map((v) => `- \`${v}\``).join("\n")} diff --git a/scripts/generate-json-schemas.ts b/scripts/generate-json-schemas.ts index 5ad59b283..e66665cf0 100644 --- a/scripts/generate-json-schemas.ts +++ b/scripts/generate-json-schemas.ts @@ -140,7 +140,7 @@ const ioSchema = { connection: { type: "string", description: - "Physical connector (e.g., 1/4-inch, xlr, usb-c). Describes the plug/jack, not the signal type. Known values are in schema/io-connections.yaml — unknown values produce warnings.", + "Physical connector (e.g., 1/4-inch, xlr, usb-c). Describes the plug/jack, not the signal type. Known values are in schema/io-connections.yaml — an unknown value fails validation (E130).", }, connectorDetail: { type: "array", items: { type: "string" } }, maxConnections: { type: "number" }, diff --git a/scripts/generate-patch.ts b/scripts/generate-patch.ts index 1d6182eee..f7e6fbb07 100644 --- a/scripts/generate-patch.ts +++ b/scripts/generate-patch.ts @@ -28,7 +28,7 @@ import Database from "better-sqlite3"; import { parse as parseYaml } from "yaml"; import type { Change } from "./lib/types.js"; -import { DATA_DIR, escapeSQL, OUTPUT_DIR } from "./lib/utils.js"; +import { DATA_DIR, escapeSQL, OUTPUT_DIR, readRegularFile } from "./lib/utils.js"; const PATCHES_DIR = path.join(OUTPUT_DIR, "patches"); const DEFAULT_DB = path.join(OUTPUT_DIR, "catalog.sqlite"); @@ -533,7 +533,7 @@ export function insertStatements( function readEntryId(category: Change["category"], slug: string): string | null { try { const file = path.join(DATA_DIR, category, `${slug}.yaml`); - return (parseYaml(fs.readFileSync(file, "utf-8")) as { id?: string }).id ?? null; + return (parseYaml(readRegularFile(file)) as { id?: string }).id ?? null; } catch { return null; } diff --git a/scripts/io-quality.ts b/scripts/io-quality.ts index 5cbcece8b..64eeaa0e6 100644 --- a/scripts/io-quality.ts +++ b/scripts/io-quality.ts @@ -17,8 +17,8 @@ * raw material the setup graph needs (densest devices first). * * The report is advisory triage: heuristics favor recall, so a human reviews - * the worklist. The high-precision subset is enforced separately as warning - * W128 in `pnpm validate`. + * the worklist. The high-precision subset is enforced separately as error + * E131 in `pnpm validate`. * * Usage: * pnpm io-quality # console report diff --git a/scripts/lib/duplicates.ts b/scripts/lib/duplicates.ts new file mode 100644 index 000000000..489d29a01 --- /dev/null +++ b/scripts/lib/duplicates.ts @@ -0,0 +1,111 @@ +/** + * Entries that collide with another entry of the same collection. + * + * Two kinds of collision, both exact after normalisation and neither + * fuzzy. A product catalog is full of model-number families + * ("Bassysm-F" beside "Bassysm-J") that edit-distance matching would + * flag by the thousand, so the question asked here is the narrow one: + * is this the same product filed twice? + * + * - A **name** collides when the collection, the manufacturer and the + * normalised name all agree ("Pro-Q 3" and "ProQ3" under fabfilter). + * - A **url** collides when two entries in a collection point at the + * same page, read without scheme, `www.` or a trailing slash. + * + * `pnpm validate` reports each group once (W134, W135) and + * `pnpm dataset:audit` carries the same groups into the monthly issue, + * from this one implementation so the two never disagree about a pair. + * Reviewers used to be asked to spot both by eye, one import PR at a + * time, which is how 149 url groups came to sit on `main`. + */ + +export interface DuplicateCandidate { + collection: string; + /** Repo-relative path, the way the report names it. */ + file: string; + name?: string; + manufacturer?: string; + url?: string; +} + +export interface DuplicateGroup { + collection: string; + /** The value the group shares, as the first member spells it. */ + value: string; + entries: T[]; +} + +/** Collapse a display name to a comparable key (lowercase, alphanumerics only). */ +export function normalizeName(name: string): string { + return ( + name + .toLowerCase() + // A "+" marks a distinct SKU, so preserve it as a "plus" token instead of + // stripping it. This covers both a trailing "+" (ProFX10v3 vs ProFX10v3+, + // Prime 4 vs Prime 4+) and an internal "+" that precedes more of the name + // (Vision+ Console vs Vision Console, SPS-1UW+ MKII vs SPS-1UW MKII). + .replace(/\+/g, "plus") + .replace(/[^a-z0-9]+/g, "") + ); +} + +/** A url without the parts that vary between two spellings of one page. */ +export function normalizeUrl(url: string): string { + return url + .trim() + .replace(/^https?:\/\//i, "") + .replace(/^www\./i, "") + .replace(/\/+$/, "") + .toLowerCase(); +} + +function groupBy( + entries: readonly T[], + keyOf: (entry: T) => string | undefined, + sharedValue: (entry: T) => string +): DuplicateGroup[] { + const buckets = new Map(); + for (const entry of entries) { + const key = keyOf(entry); + if (!key) continue; + const list = buckets.get(`${entry.collection}\u0000${key}`) ?? []; + list.push(entry); + buckets.set(`${entry.collection}\u0000${key}`, list); + } + const groups: DuplicateGroup[] = []; + for (const group of buckets.values()) { + if (group.length < 2) continue; + groups.push({ collection: group[0].collection, value: sharedValue(group[0]), entries: group }); + } + return groups; +} + +/** + * Groups sharing collection, manufacturer and normalised name. An entry + * with no manufacturer (a manufacturer file) never groups, because the + * name alone is not enough to say two files are one product. + */ +export function findDuplicateNameGroups( + entries: readonly T[] +): DuplicateGroup[] { + return groupBy( + entries, + (entry) => { + if (!entry.name || !entry.manufacturer) return undefined; + const norm = normalizeName(entry.name); + return norm ? `${entry.manufacturer}\u0000${norm}` : undefined; + }, + (entry) => entry.name ?? "" + ); +} + +/** Groups sharing a collection and a normalised `url`. */ +export function findDuplicateUrlGroups( + entries: readonly T[] +): DuplicateGroup[] { + return groupBy( + entries, + (entry) => (entry.url ? normalizeUrl(entry.url) || undefined : undefined), + (entry) => entry.url ?? "" + ); +} diff --git a/scripts/lib/entry-id.ts b/scripts/lib/entry-id.ts new file mode 100644 index 000000000..b76266176 --- /dev/null +++ b/scripts/lib/entry-id.ts @@ -0,0 +1,13 @@ +/** + * The shape of an entry's `id`. + * + * `pnpm assign-ids` writes a 21-character nanoid (URL-safe alphabet: + * letters, digits, `-` and `_`), and nothing else is meant to write the + * field at all. Until this existed the validator checked only that the + * value was a non-empty string, so a hand-typed `id: 1`, a slug pasted + * into the field or a truncated paste validated, built and shipped, and + * Studio's `supersedes` and image lookups keyed on it. The corpus was + * clean when the pattern was introduced, which is why it is an error + * (E129) rather than a staged warning. + */ +export const ENTRY_ID_PATTERN = /^[A-Za-z0-9_-]{21}$/; diff --git a/scripts/lib/error-codes.ts b/scripts/lib/error-codes.ts index 4b5b383df..4525126ad 100644 --- a/scripts/lib/error-codes.ts +++ b/scripts/lib/error-codes.ts @@ -49,6 +49,9 @@ export enum ValidationErrorCode { E126_CONTROL_CHARACTER = "E126", E127_INVALID_PRICE_AMOUNT = "E127", E128_UNSAFE_URL_SCHEME = "E128", + E129_INVALID_ID_FORMAT = "E129", + E130_UNKNOWN_IO_CONNECTION = "E130", + E131_IO_COLLAPSED_JACKS = "E131", E199_VALIDATION_ERROR = "E199", // Reference errors (E2xx) @@ -58,6 +61,7 @@ export enum ValidationErrorCode { E203_PARENT_COMPANY_NOT_FOUND = "E203", E204_IO_TRANSLATION_MISMATCH = "E204", E206_UNKNOWN_COMPATIBLE_WITH = "E206", + E207_DUPLICATE_ID = "E207", // Content errors (E3xx) E300_INVALID_MARKDOWN = "E300", @@ -73,17 +77,17 @@ export enum ValidationErrorCode { E402_DUPLICATE_IDENTIFIER = "E402", // Advisory warnings (W1xx) — non-blocking - W120_UNKNOWN_IO_TYPE = "W120", - W121_UNKNOWN_IO_CONNECTION = "W121", W124_DUPLICATE_URL = "W124", W125_MANUFACTURER_URL_IN_LINKS = "W125", W126_SPECS_OVERLAP = "W126", W127_MISSING_SEARCH_TERMS = "W127", - W128_IO_COMBINE_CANDIDATE = "W128", W129_MANUFACTURER_IN_NAME = "W129", W130_NAME_TAGLINE = "W130", W131_PRICE_TERM_MISSING = "W131", W133_UNVERIFIED_FREE_PRICE = "W133", + W134_DUPLICATE_NAME_IN_COLLECTION = "W134", + W135_DUPLICATE_URL_IN_COLLECTION = "W135", + W136_MISSING_DESCRIPTION = "W136", } // ============================================================================= @@ -254,6 +258,18 @@ const ERROR_INFO: Record = { title: "URL scheme is not http or https", anchor: "e128-url-scheme-is-not-http-or-https", }, + [ValidationErrorCode.E129_INVALID_ID_FORMAT]: { + title: "Id is not a 21-character nanoid", + anchor: "e129-id-is-not-a-21-character-nanoid", + }, + [ValidationErrorCode.E130_UNKNOWN_IO_CONNECTION]: { + title: "Unknown IO connection", + anchor: "e130-unknown-io-connection", + }, + [ValidationErrorCode.E131_IO_COLLAPSED_JACKS]: { + title: "IO entry collapses several physical jacks", + anchor: "e131-io-entry-collapses-several-physical-jacks", + }, [ValidationErrorCode.E199_VALIDATION_ERROR]: { title: "Validation error", anchor: "e199-validation-error", @@ -284,6 +300,10 @@ const ERROR_INFO: Record = { title: "Unknown compatibleWith reference", anchor: "e206-unknown-compatiblewith-reference", }, + [ValidationErrorCode.E207_DUPLICATE_ID]: { + title: "Id claimed by two entries", + anchor: "e207-id-claimed-by-two-entries", + }, // Content errors [ValidationErrorCode.E300_INVALID_MARKDOWN]: { @@ -326,14 +346,6 @@ const ERROR_INFO: Record = { }, // Advisory warnings - [ValidationErrorCode.W120_UNKNOWN_IO_TYPE]: { - title: "Unknown IO type", - anchor: "w120-unknown-io-type", - }, - [ValidationErrorCode.W121_UNKNOWN_IO_CONNECTION]: { - title: "Unknown IO connection", - anchor: "w121-unknown-io-connection", - }, [ValidationErrorCode.W124_DUPLICATE_URL]: { title: "Duplicate URL", anchor: "w124-duplicate-url", @@ -350,10 +362,6 @@ const ERROR_INFO: Record = { title: "Entry would benefit from searchTerms", anchor: "w127-missing-search-terms", }, - [ValidationErrorCode.W128_IO_COMBINE_CANDIDATE]: { - title: "IO entry may combine multiple physical jacks", - anchor: "w128-io-entry-may-combine-multiple-physical-jacks", - }, [ValidationErrorCode.W129_MANUFACTURER_IN_NAME]: { title: "Manufacturer name duplicated in product name", anchor: "w129-manufacturer-name-duplicated-in-product-name", @@ -370,6 +378,18 @@ const ERROR_INFO: Record = { title: "Free price is missing source or asOf", anchor: "w133-free-price-is-missing-source-or-asof", }, + [ValidationErrorCode.W134_DUPLICATE_NAME_IN_COLLECTION]: { + title: "Name shared with another entry of the same maker", + anchor: "w134-name-shared-with-another-entry-of-the-same-maker", + }, + [ValidationErrorCode.W135_DUPLICATE_URL_IN_COLLECTION]: { + title: "URL shared with another entry in the collection", + anchor: "w135-url-shared-with-another-entry-in-the-collection", + }, + [ValidationErrorCode.W136_MISSING_DESCRIPTION]: { + title: "Hardware or accessory entry has no description", + anchor: "w136-hardware-or-accessory-entry-has-no-description", + }, }; // ============================================================================= diff --git a/scripts/lib/io-heuristics.ts b/scripts/lib/io-heuristics.ts index 22855a4a2..2a5de9f42 100644 --- a/scripts/lib/io-heuristics.ts +++ b/scripts/lib/io-heuristics.ts @@ -1,7 +1,7 @@ /** * Shared IO data-quality heuristics. * - * Used by both `validate.ts` (warning W128) and the `io-quality` report so the + * Used by both `validate.ts` (error E131) and the `io-quality` report so the * CI signal and the triage report stay in lockstep. */ diff --git a/scripts/lib/utils.ts b/scripts/lib/utils.ts index 17fd952ce..91bfa6e74 100644 --- a/scripts/lib/utils.ts +++ b/scripts/lib/utils.ts @@ -180,13 +180,22 @@ export function parseErrorPath(pathStr: string): (string | number)[] { } /** - * Get all YAML files in a directory + * The YAML files in a collection directory. + * + * Regular files only, read off the dirent rather than the name: a + * directory listing is a list of names, and a name says nothing about + * what is at it. A committed `data/software/x.yaml -> /dev/zero` is a + * symlink, not a catalog file, so it is not listed, and the read that + * follows still goes through `readRegularFile`, because a listing is a + * statement about then and the read is now. */ export function getYamlFiles(dir: string): string[] { if (!fs.existsSync(dir)) return []; return ( fs - .readdirSync(dir) + .readdirSync(dir, { withFileTypes: true }) + .filter((entry) => entry.isFile()) + .map((entry) => entry.name) .filter((f) => f.endsWith(".yaml") || f.endsWith(".yml")) // Sorted so the SQLite build is deterministic: readdir order is // filesystem-dependent, and insertion order feeds autoincrement ids. diff --git a/scripts/schema.sql b/scripts/schema.sql index a4cf0c950..0f6eeb20c 100644 --- a/scripts/schema.sql +++ b/scripts/schema.sql @@ -752,9 +752,17 @@ CREATE INDEX idx_hardware_io_translations ON hardware_io_translations(hardware_i -- FULL-TEXT SEARCH -- ============================================================================= +-- `id` is stored but UNINDEXED in every *_fts table: it is the join key +-- back to the base row (Studio selects it and filters `id = x.id`), never +-- a thing a person searches for, and indexing a 21-character nanoid per +-- row only grew the index and let a search for a name match an id that +-- happened to contain it. The MATCH interface is unchanged: the column +-- is still there, in the same position, so the bm25() weights Studio +-- passes per column still line up. + -- Manufacturer FTS index CREATE VIRTUAL TABLE IF NOT EXISTS manufacturers_fts USING fts5( - id, + id UNINDEXED, name, company_name, description, @@ -764,7 +772,7 @@ CREATE VIRTUAL TABLE IF NOT EXISTS manufacturers_fts USING fts5( -- Content FTS index CREATE VIRTUAL TABLE IF NOT EXISTS content_fts USING fts5( - id, + id UNINDEXED, name, manufacturer_name, categories, @@ -777,7 +785,7 @@ CREATE VIRTUAL TABLE IF NOT EXISTS content_fts USING fts5( -- Note: Not using content='' (contentless mode) because we need to retrieve -- the id column to join back to the software table for full results CREATE VIRTUAL TABLE IF NOT EXISTS software_fts USING fts5( - id, + id UNINDEXED, name, manufacturer_name, categories, @@ -790,7 +798,7 @@ CREATE VIRTUAL TABLE IF NOT EXISTS software_fts USING fts5( -- Note: Not using content='' (contentless mode) because we need to retrieve -- the id column to join back to the hardware table for full results CREATE VIRTUAL TABLE IF NOT EXISTS hardware_fts USING fts5( - id, + id UNINDEXED, name, manufacturer_name, categories, @@ -801,7 +809,7 @@ CREATE VIRTUAL TABLE IF NOT EXISTS hardware_fts USING fts5( -- Accessories FTS index CREATE VIRTUAL TABLE IF NOT EXISTS accessories_fts USING fts5( - id, + id UNINDEXED, name, manufacturer_name, categories, diff --git a/scripts/validate-translations.ts b/scripts/validate-translations.ts index 09c020cdf..6751e6fa0 100644 --- a/scripts/validate-translations.ts +++ b/scripts/validate-translations.ts @@ -19,7 +19,7 @@ import type { Manufacturer, Software, } from "./lib/types.js"; -import { DATA_DIR, getYamlFiles, loadYamlFile, SCHEMA_DIR } from "./lib/utils.js"; +import { DATA_DIR, getYamlFiles, loadYamlFile, readRegularFile, SCHEMA_DIR } from "./lib/utils.js"; // ============================================================================= // LOAD APPROVED LOCALES @@ -280,7 +280,7 @@ function validateTranslations(): TranslationValidationResult { const manufacturerFiles = getYamlFiles(path.join(DATA_DIR, "manufacturers")); for (const file of manufacturerFiles) { try { - const content = fs.readFileSync(file, "utf-8"); + const content = readRegularFile(file); const data = parseYaml(content) as Manufacturer; if (data.translations) { @@ -309,7 +309,7 @@ function validateTranslations(): TranslationValidationResult { const softwareFiles = getYamlFiles(path.join(DATA_DIR, "software")); for (const file of softwareFiles) { try { - const content = fs.readFileSync(file, "utf-8"); + const content = readRegularFile(file); const data = parseYaml(content) as Software; if (data.translations) { @@ -338,7 +338,7 @@ function validateTranslations(): TranslationValidationResult { const contentDataFiles = getYamlFiles(path.join(DATA_DIR, "content")); for (const file of contentDataFiles) { try { - const rawContent = fs.readFileSync(file, "utf-8"); + const rawContent = readRegularFile(file); const data = parseYaml(rawContent) as Content; if (data.translations) { @@ -367,7 +367,7 @@ function validateTranslations(): TranslationValidationResult { const hardwareFiles = getYamlFiles(path.join(DATA_DIR, "hardware")); for (const file of hardwareFiles) { try { - const content = fs.readFileSync(file, "utf-8"); + const content = readRegularFile(file); const data = parseYaml(content) as Hardware; if (data.translations) { @@ -396,7 +396,7 @@ function validateTranslations(): TranslationValidationResult { const accessoryFiles = getYamlFiles(path.join(DATA_DIR, "accessories")); for (const file of accessoryFiles) { try { - const rawContent = fs.readFileSync(file, "utf-8"); + const rawContent = readRegularFile(file); const data = parseYaml(rawContent) as Accessory; if (data.translations) { diff --git a/scripts/validate.ts b/scripts/validate.ts index c773b4a06..065712fff 100644 --- a/scripts/validate.ts +++ b/scripts/validate.ts @@ -13,6 +13,13 @@ import { parse as parseYaml } from "yaml"; import { z } from "zod"; import { looksLikeAcronymName } from "./lib/acronym-exclusions.js"; import { findControlCharacters, formatControlCharacterPath } from "./lib/control-characters.js"; +import { + type DuplicateCandidate, + findDuplicateNameGroups, + findDuplicateUrlGroups, + normalizeUrl, +} from "./lib/duplicates.js"; +import { ENTRY_ID_PATTERN } from "./lib/entry-id.js"; import { getDocsUrl, ValidationErrorCode } from "./lib/error-codes.js"; import { findComponentIdentifierRepeats, @@ -59,6 +66,7 @@ import { loadYamlFile, loadYamlFileWithPositions, parseErrorPath, + readRegularFile, SCHEMA_DIR, slugify, } from "./lib/utils.js"; @@ -110,6 +118,27 @@ export function isValidCategory(cat: string): boolean { return ALL_VALID_CATEGORY_INPUTS.has(cat); } +/** + * `id` as `pnpm assign-ids` writes it (E129). Optional here and required + * in the build, the same split `io[].key` takes: validate.yml runs on a + * head commit the assign-ids patch has not landed on yet. + */ +function createIdValidator() { + return z + .string() + .check((ctx) => { + if (ENTRY_ID_PATTERN.test(ctx.value)) return; + ctx.issues.push({ + code: "custom", + message: + `Invalid id '${ctx.value}': an id is the 21-character nanoid pnpm assign-ids writes ` + + "(letters, digits, '-' and '_'). Remove a hand-written id and let CI assign one.", + input: ctx.value, + }); + }) + .optional(); +} + // Capabilities are validated strictly (E119) rather than advisorily. The whole // value of the field is that two entries' lists are comparable, and a typo or a // near-miss synonym silently breaks comparability while looking populated. @@ -195,6 +224,12 @@ export function getErrorCodeFromZodIssue(issue: { const path = issue.path.join("."); const message = issue.message.toLowerCase(); + // The entry id (E129) is keyed to its path: the message names the + // value, which could carry any word the branches below key on. + if (path === "id") { + return ValidationErrorCode.E129_INVALID_ID_FORMAT; + } + // Name hygiene errors if (message.includes("name artifact")) { return ValidationErrorCode.E118_NAME_ARTIFACT; @@ -261,6 +296,12 @@ export function getErrorCodeFromZodIssue(issue: { if (message.includes("invalid io type")) { return ValidationErrorCode.E117_INVALID_IO_TYPE; } + if (message.includes("unknown io connection")) { + return ValidationErrorCode.E130_UNKNOWN_IO_CONNECTION; + } + if (message.includes("several physical jacks")) { + return ValidationErrorCode.E131_IO_COLLAPSED_JACKS; + } // Layout hint errors (E122 to E125). The cross-port rules carry a marker // phrase; a hint that fails the number check is keyed to its path so a @@ -391,20 +432,13 @@ export function validateMarkdown(content: string): { valid: boolean; error?: str } } -// Helper to normalize string or string[] to a single markdown string -export function normalizeToMarkdownString(value: string | string[]): string { - if (Array.isArray(value)) { - // Join array items with double newlines (paragraph breaks) - return value.join("\n\n"); - } - return value; -} - -// Zod schema for markdown content validation (accepts string or string[]) +// Zod schema for markdown content validation. A string only: CLAUDE.md +// has always asked for block scalars rather than YAML arrays in `details` +// and `specs`, and the corpus carried no array when the union that +// accepted one was dropped, so an array is the type error E101. const MarkdownSchema = z - .union([z.string(), z.array(z.string())]) + .string() .optional() - .transform((val) => (val ? normalizeToMarkdownString(val) : val)) .check((ctx) => { if (!ctx.value) return; const result = validateMarkdown(ctx.value); @@ -660,7 +694,21 @@ const IOSchema = z ctx.issues.push({ code: "custom", message, input: ctx.value }); } }), - connection: z.string(), + // Closed vocabulary like `type` (E117). This was the advisory W121 + // until the corpus carried one unknown value (`sc`, the optical MADI + // connector, now in the schema), so a new connector lands in + // schema/io-connections.yaml in the same PR as the entry. + connection: z.string().check((ctx) => { + if (KNOWN_IO_CONNECTIONS.has(ctx.value)) return; + const suggestion = findClosestMatch(ctx.value, KNOWN_IO_CONNECTIONS); + let message = `Unknown IO connection '${ctx.value}'.`; + if (suggestion) { + message += ` Did you mean '${suggestion}'?`; + } + message += " If this is a real connector, add it to schema/io-connections.yaml."; + message += " If it names a signal, it belongs in `type`."; + ctx.issues.push({ code: "custom", message, input: ctx.value }); + }), connectorDetail: z.array(z.string()).optional(), maxConnections: z.number(), position: z @@ -791,6 +839,7 @@ const createNameValidator = () => // ============================================================================= const ManufacturerSchema = z.object({ + id: createIdValidator(), name: createNameValidator(), companyName: z.string().optional(), parentCompany: z.string().optional(), @@ -802,6 +851,7 @@ const ManufacturerSchema = z.object({ const SoftwareSchema = z .object({ + id: createIdValidator(), name: createNameValidator(), manufacturer: z.string().min(1, "Manufacturer reference is required"), categories: z @@ -870,6 +920,7 @@ const SoftwareSchema = z const HardwareSchema = z .object({ + id: createIdValidator(), name: createNameValidator(), manufacturer: z.string().min(1, "Manufacturer reference is required"), categories: z @@ -927,6 +978,27 @@ const HardwareSchema = z } }); }) + .check((ctx) => { + // One io entry per physical jack: a single-jack connection claiming + // more than one connection is several jacks collapsed into one entry, + // the mistake bulk imports made most. The heuristic (shared with + // `pnpm io-quality`) exempts named aggregates and patchbay rows. It + // was the advisory W128 until the corpus carried no hit. + const data = ctx.value; + if (!data.io || data.io.length === 0) return; + data.io.forEach((port, i) => { + if (!isIoCombineCandidate(port, { primaryCategory: data.primaryCategory })) return; + ctx.issues.push({ + code: "custom", + message: + `io entry '${port.name}' sets maxConnections ${port.maxConnections} on a single-jack ` + + `connection ('${port.connection}'), which collapses several physical jacks into one ` + + "entry. Give each jack its own io entry with maxConnections: 1.", + path: ["io", i, "maxConnections"], + input: port, + }); + }); + }) .check((ctx) => { // IO keys must be unique within the entry (they identify ports for // Studio setup edges — see io-keys.ts). @@ -998,6 +1070,7 @@ const HardwareSchema = z // Content schema: like Software but without formats, platforms, identifiers const ContentSchema = z .object({ + id: createIdValidator(), name: createNameValidator(), manufacturer: z.string().min(1, "Manufacturer reference is required"), categories: z @@ -1044,6 +1117,7 @@ const ContentSchema = z // Accessory schema: like Hardware but without io, variants const AccessorySchema = z .object({ + id: createIdValidator(), name: createNameValidator(), manufacturer: z.string().min(1, "Manufacturer reference is required"), categories: z @@ -1109,9 +1183,9 @@ interface DataWithOptionalFields { supersedes?: string; identifiers?: Record; componentIdentifiers?: Record; - description?: string | string[]; - details?: string | string[]; - specs?: string | string[]; + description?: string; + details?: string; + specs?: string; } /** @@ -1443,9 +1517,7 @@ export function validateFile( // Matches text cut mid-word ("featur...") or ending with generic ellipsis, // but excludes legitimate trailing phrases like "and more..." or "etc..." if (data.description) { - const descText = Array.isArray(data.description) - ? data.description.join("\n\n") - : data.description; + const descText = data.description; if (/\.{3,}\s*$/.test(descText) && !/(?:more|etc|others|so on)\.{3,}\s*$/i.test(descText)) { const line = getLineForPath(document, lineCounter, ["description"]); const errorCode = ValidationErrorCode.E304_TRUNCATED_CONTENT; @@ -1553,7 +1625,7 @@ export function checkCompatibleWith( export interface WarningContext { name?: string; primaryCategory?: string; - io?: Array<{ name: string; type: string; connection: string; maxConnections?: number }>; + description?: string; url?: string; links?: Array<{ url: string }>; manufacturer?: string; @@ -1625,35 +1697,22 @@ export function collectWarnings( } } - // io.type is not checked here: it is a hard error (E117) via the Zod IOSchema, - // so an unknown value fails validation outright. io.connection remains advisory. - if (Array.isArray(data.io)) { - for (let i = 0; i < data.io.length; i++) { - const io = data.io[i]; - - if (io.connection && !KNOWN_IO_CONNECTIONS.has(io.connection)) { - const line = getLineForPath(document, lineCounter, ["io", i, "connection"]); - warnings.push({ - code: ValidationErrorCode.W121_UNKNOWN_IO_CONNECTION, - message: `Unknown IO connection '${io.connection}' on '${io.name}'. Consider adding to schema/io-connections.yaml if valid.`, - path: `io[${i}].connection`, - line: line ?? undefined, - }); - } - - // A single-jack connection carries one physical link; maxConnections>1 - // usually means several jacks were collapsed into one entry (aggregates - // like "All Slots" are excluded). - if (isIoCombineCandidate(io, { primaryCategory: data.primaryCategory })) { - const line = getLineForPath(document, lineCounter, ["io", i, "maxConnections"]); - warnings.push({ - code: ValidationErrorCode.W128_IO_COMBINE_CANDIDATE, - message: `IO '${io.name}' sets maxConnections ${io.maxConnections} on a single-jack connection ('${io.connection}'). If this is several physical jacks, give each its own io entry with maxConnections: 1.`, - path: `io[${i}].maxConnections`, - line: line ?? undefined, - }); - } - } + // W136: CLAUDE.md requires `description` on hardware and accessories, + // and the schema never did. Staged as a warning while 33 entries on + // `main` carry none; io.type and io.connection are hard errors (E117, + // E130) in the Zod IOSchema and are not repeated here. + const collection = path.basename(path.dirname(filePath)); + if ( + (collection === "hardware" || collection === "accessories") && + !(typeof data.description === "string" && data.description.trim()) + ) { + const line = getLineForPath(document, lineCounter, ["name"]); + warnings.push({ + code: ValidationErrorCode.W136_MISSING_DESCRIPTION, + message: `A ${collection} entry needs a description (CLAUDE.md "Data Entry Format"). Write one sentence from the maker's page.`, + path: "description", + line: line ?? undefined, + }); } // Check for duplicate URLs within the file @@ -1687,15 +1746,9 @@ export function collectWarnings( if (data.manufacturer && manufacturerUrlMap) { const mfrUrl = manufacturerUrlMap.get(data.manufacturer); if (mfrUrl) { - const normalize = (u: string) => - u - .replace(/^https?:\/\//, "") - .replace(/^www\./, "") - .replace(/\/+$/, "") - .toLowerCase(); - const normalizedMfr = normalize(mfrUrl); - - if (data.url && normalize(data.url) === normalizedMfr) { + const normalizedMfr = normalizeUrl(mfrUrl); + + if (data.url && normalizeUrl(data.url) === normalizedMfr) { const line = getLineForPath(document, lineCounter, ["url"]); warnings.push({ code: ValidationErrorCode.W125_MANUFACTURER_URL_IN_LINKS, @@ -1708,7 +1761,7 @@ export function collectWarnings( if (Array.isArray(data.links)) { for (let i = 0; i < data.links.length; i++) { const linkUrl = data.links[i].url; - if (linkUrl && normalize(linkUrl) === normalizedMfr) { + if (linkUrl && normalizeUrl(linkUrl) === normalizedMfr) { const line = getLineForPath(document, lineCounter, ["links", i, "url"]); warnings.push({ code: ValidationErrorCode.W125_MANUFACTURER_URL_IN_LINKS, @@ -1920,7 +1973,7 @@ function validate(): ValidationResult { const slug = path.basename(file, path.extname(file)); allManufacturers.add(slug); try { - const content = fs.readFileSync(file, "utf-8"); + const content = readRegularFile(file); const mfrData = parseYaml(content) as { url?: string; name?: string }; if (mfrData.url) manufacturerUrlMap.set(slug, mfrData.url); if (mfrData.name) manufacturerNameMap.set(slug, mfrData.name); @@ -1941,17 +1994,33 @@ function validate(): ValidationResult { allHardwareSlugs.add(path.basename(file, path.extname(file))); } + // Every product's name, manufacturer and url, for the cross-file + // duplicate groups (W134, W135) reported after the per-file pass. + type DuplicateFields = { name?: string; manufacturer?: string; url?: string }; + const duplicateCandidates: DuplicateCandidate[] = []; + const noteCandidate = (collection: string, file: string, data: DuplicateFields) => { + duplicateCandidates.push({ + collection, + file: path.relative(process.cwd(), file), + name: typeof data.name === "string" ? data.name : undefined, + manufacturer: typeof data.manufacturer === "string" ? data.manufacturer : undefined, + url: typeof data.url === "string" ? data.url : undefined, + }); + }; + // Collect software IDs and supersedes relationships, and every entry's // identifiers for the cross-file E402 check below const identifierOwners: IdentifierOwner[] = []; for (const file of softwareFiles) { const slug = path.basename(file, path.extname(file)); try { - const content = fs.readFileSync(file, "utf-8"); + const content = readRegularFile(file); const data = parseYaml(content) as { id?: string; supersedes?: string } & Omit< IdentifierOwner, "slug" - >; + > & + DuplicateFields; + noteCandidate("software", file, data); identifierOwners.push({ slug, identifiers: data.identifiers, @@ -1973,8 +2042,9 @@ function validate(): ValidationResult { for (const file of contentFiles) { const slug = path.basename(file, path.extname(file)); try { - const rawContent = fs.readFileSync(file, "utf-8"); - const data = parseYaml(rawContent) as { id?: string; supersedes?: string }; + const rawContent = readRegularFile(file); + const data = parseYaml(rawContent) as { id?: string; supersedes?: string } & DuplicateFields; + noteCandidate("content", file, data); if (data.id) { contentIds.add(data.id); contentIdToSlug.set(data.id, slug); @@ -1991,8 +2061,9 @@ function validate(): ValidationResult { for (const file of hardwareFiles) { const slug = path.basename(file, path.extname(file)); try { - const content = fs.readFileSync(file, "utf-8"); - const data = parseYaml(content) as { id?: string; supersedes?: string }; + const content = readRegularFile(file); + const data = parseYaml(content) as { id?: string; supersedes?: string } & DuplicateFields; + noteCandidate("hardware", file, data); if (data.id) { hardwareIds.add(data.id); hardwareIdToSlug.set(data.id, slug); @@ -2009,8 +2080,9 @@ function validate(): ValidationResult { for (const file of accessoryFiles) { const slug = path.basename(file, path.extname(file)); try { - const rawContent = fs.readFileSync(file, "utf-8"); - const data = parseYaml(rawContent) as { id?: string; supersedes?: string }; + const rawContent = readRegularFile(file); + const data = parseYaml(rawContent) as { id?: string; supersedes?: string } & DuplicateFields; + noteCandidate("accessories", file, data); if (data.id) { accessoryIds.add(data.id); accessoryIdToSlug.set(data.id, slug); @@ -2149,6 +2221,45 @@ function validate(): ValidationResult { } } + // W134 and W135: the same product filed twice, by name or by url. Each + // group is reported once, on its first file, naming the others, so the + // count in the summary is a count of pairs to look at rather than of + // files. Advisory while `main` carries them (66 name groups and 149 + // url groups when the check was written); the monthly dataset audit + // carries the same groups into its issue. + for (const group of findDuplicateNameGroups(duplicateCandidates)) { + const [first, ...rest] = group.entries; + warnings.push({ + file: first.file, + warnings: [ + { + code: ValidationErrorCode.W134_DUPLICATE_NAME_IN_COLLECTION, + message: + `'${group.value}' is also the name of ${rest.map((e) => e.file).join(", ")} under the ` + + "same manufacturer. Merge the entries, fold a cosmetic variant into `variants`, or " + + "disambiguate the names.", + path: "name", + }, + ], + }); + } + for (const group of findDuplicateUrlGroups(duplicateCandidates)) { + const [first, ...rest] = group.entries; + warnings.push({ + file: first.file, + warnings: [ + { + code: ValidationErrorCode.W135_DUPLICATE_URL_IN_COLLECTION, + message: + `url '${group.value}' is also the url of ${rest.map((e) => e.file).join(", ")}. ` + + "Two entries on one page are usually one product filed twice; otherwise give each " + + "its own product page.", + path: "url", + }, + ], + }); + } + // Check for cycles in supersedes chains (software) for (const [id, _supersedes] of softwareSupersedesMap) { const cycle = detectSupersedeCycle(id, softwareSupersedesMap, softwareIdToSlug); @@ -2267,13 +2378,18 @@ function validateIds(): IdValidationResult { "accessories", ]; + // One map across every collection (E207). Product images are served + // from R2 keyed by id alone, and `pnpm assign-ids` draws from one pool, + // so an id shared by a hardware entry and a manufacturer is a collision + // even though the two tables never meet. + const seenIds = new Map(); // id -> collection/slug + for (const collection of collections) { const files = getYamlFiles(path.join(DATA_DIR, collection)); - const seenIds = new Map(); // id -> slug for (const file of files) { try { - const content = fs.readFileSync(file, "utf-8"); + const content = readRegularFile(file); const data = parseYaml(content) as { id?: unknown }; const slug = path.basename(file, path.extname(file)); @@ -2289,11 +2405,12 @@ function validateIds(): IdValidationResult { // Check for duplicates if (seenIds.has(data.id)) { errors.push( - `${collection}: duplicate id '${data.id}' in '${seenIds.get(data.id)}' and '${slug}'` + `${ValidationErrorCode.E207_DUPLICATE_ID}: id '${data.id}' is claimed by ` + + `'${seenIds.get(data.id)}' and '${collection}/${slug}' (${getDocsUrl(ValidationErrorCode.E207_DUPLICATE_ID)})` ); stats.duplicates++; } - seenIds.set(data.id, slug); + seenIds.set(data.id, `${collection}/${slug}`); stats.withIds++; } else { stats.withoutIds++;