From a3c985c907a55546aa9735803bcb4519e4342c2b Mon Sep 17 00:00:00 2001 From: Sinduri Guntupalli Date: Mon, 7 Sep 2026 11:40:42 +0200 Subject: [PATCH 1/3] fix(sync): carry contributors from challenges repo, warn on long meta descriptions The Sync Adventure workflow failed on adventure-accessibility-nightmare with two unrelated schema errors. Both are fixed here. **meta_description length no longer fails the build** - Dropped `.max(160)` from the adventure and level schemas - `warnIfMetaDescriptionLong` logs the over-run instead, called from `renderLevel` and the adventure transform - Search engines truncate a long description rather than reject it, so this was never worth failing a whole sync run over **Contributors now sync from the challenges repo** - The adventure designer is read from `contributor:` in `docs/index.yaml`. The sync previously kept only one already hand-written into the website YAML, so a freshly synced adventure got none - The challenge builder already flowed through from the level YAML, but unfiltered. `pickContributor` now keeps only the four fields the strict content schema accepts, on both paths - A contributor already credited on the website wins over the upstream value at both levels, and `mergeLevels` warns when the two diverge rather than dropping the divergence silently The missing designer alongside levels naming their own builder is what tripped `creditIntegrityError` and produced the second error. - Tests: `pickContributor`, `buildLevel` and `mergeLevels` credit rules - Docs: ADVENTURES.md field and preservation tables, sync PR checklist - Corrected two comments claiming the sync deliberately omits `contributor` Signed-off-by: Sinduri Guntupalli --- .github/workflows/sync-adventure.yml | 3 +- ADVENTURES.md | 19 +++-- scripts/sync-adventure.mjs | 57 ++++++++++++-- src/content.config.ts | 26 ++++++- src/lib/adventure-credit.ts | 5 +- src/test/lib/adventure-credit.test.ts | 3 +- src/test/scripts/sync-adventure.test.ts | 98 ++++++++++++++++++++++++- 7 files changed, 190 insertions(+), 21 deletions(-) diff --git a/.github/workflows/sync-adventure.yml b/.github/workflows/sync-adventure.yml index 4d143feab..c8b61462f 100644 --- a/.github/workflows/sync-adventure.yml +++ b/.github/workflows/sync-adventure.yml @@ -182,7 +182,8 @@ jobs: ### Before merging - - [ ] Add \`contributor:\` block to \`src/data/adventures/${SLUG}/adventure.yaml\` + - [ ] Check the \`contributor:\` block in \`src/data/adventures/${SLUG}/adventure.yaml\`. It is synced from + \`docs/index.yaml\` in the challenges repo; add it by hand only if the sync log says none was found. \`\`\`yaml contributor: name: "Full Name" diff --git a/ADVENTURES.md b/ADVENTURES.md index e1de41a50..a598bc821 100644 --- a/ADVENTURES.md +++ b/ADVENTURES.md @@ -40,11 +40,11 @@ The authoritative schema is in [`src/content.config.ts`](src/content.config.ts) | `emoji` | Optional | emoji character | Shown on the adventure card. The sync workflow maps it to a Lucide icon via the `EMOJI_TO_ICON` table; add the mapping there first if the emoji is new. | | `month` | **Required** | `MMM YYYY` | Three-letter uppercase abbreviation + four-digit year. Allowed: `JAN FEB MAR APR MAY JUN JUL AUG SEP OCT NOV DEC`. Validated by Zod regex; wrong format fails sync. | | `tags` | **Required** | `string[]` (min 1) | Technology/topic labels shown as filter chips. Used when the auto-generated `meta_description` falls back to name + backstory. | -| `meta_description` | **Required** | string, max 160 chars | Validated by the Zod schema; missing field fails `npm run sync`. Max 160 chars. No em dashes; no ` - ` used as a dash. | +| `meta_description` | **Required** | string, 160 chars recommended | Validated by the Zod schema; missing field fails `npm run sync`. Over 160 chars logs a warning and still builds, since search engines truncate rather than reject. Trim it before release. No em dashes; no ` - ` used as a dash. | | `story` | Optional | markdown string | Short description shown on adventure cards and at the top of the adventure page. Card views strip HTML; set:html prose uses the rendered version. | | `backstory` | Optional | `string[]` (markdown) | Thematic narrative paragraphs rendered on the adventure page. | | `overview` | Optional | `string[]` (markdown) | Technical/content summary rendered on the adventure page. | -| `contributor` | Optional | object | `name` (required), `url` (optional URL), `about` (optional markdown), `discourse_username` (optional string -- Discourse username used for avatar resolution in community leaderboards). Survives every re-sync once set. | +| `contributor` | Optional | object | `name` (required), `url` (optional URL), `about` (optional markdown), `discourse_username` (optional string -- Discourse username used for avatar resolution in community leaderboards). Copied from the challenges repo's `docs/index.yaml` when absent here; survives every re-sync once set. | | `community_category_id` | Optional | integer | Discourse category ID. Survives every re-sync once set; position is kept directly after `slug`. | | `rewards` | Optional | object | `deadline` (required inside; see format below), `eligibility` (markdown), `tiers` (array of `{label, description}`), `ranking_note` (markdown), `ranking_rules_url` (URL). | | `upcoming_levels` | Optional | object[] | Coming-soon placeholders: `{level?, name, difficulty}`. Survives re-syncs for levels not yet in the challenges repo. | @@ -82,12 +82,12 @@ Each entry in the `levels` array accepts the following fields. | `scenario` | Optional | string (markdown) | Scenario prose shown before the how-to-play steps. | | `services` | Optional | object[] | Services exposed by the devcontainer: `{name, port?, url?, credentials?, description, internal?}`. Use `port` for a bare port number or `url` for a full URL (e.g. `http://localhost:5173`). An injected "Explore the UIs" step is generated automatically when at least one non-internal service has a `port` or `url`. | | `helpful_links` | Optional | object[] | Reference links shown at the bottom of the level: `{title, url, description?}`. | -| `meta_description` | Optional | string, max 160 chars | Level-specific meta description. When absent, the generator builds one from `name`/`title` + `intro[0]` + difficulty + topics. | +| `meta_description` | Optional | string, 160 chars recommended | Level-specific meta description. When absent, the generator builds one from `name`/`title` + `intro[0]` + difficulty + topics. Over 160 chars logs a warning and still builds. | | `what_you_learn` or `learnings` | **One required** | `string[]` (min 1 when present) | Learning objectives list. A Zod `.refine()` requires at least one of the two to be set; if both are absent the build fails. | | `verification` | **Required** | object | `{command, description}` — the verification gate command and its description. | | `codespaces_machine` | Optional | `"4core"` | Machine size override for Codespaces. Only `"4core"` is accepted; other values fail the Zod schema. | | `hook` | Optional | string | Verification hook command. | -| `contributor` | Optional | object | Person who built this specific level. Same subfields as the adventure `contributor` (`name`, `url`, `about`, `discourse_username`). **When omitted, the adventure designer is credited as the builder for this level.** When set, takes precedence over the adventure designer for credit display on the level page and in community leaderboard sections. See note below. | +| `contributor` | Optional | object | Person who built this specific level. Same subfields as the adventure `contributor` (`name`, `url`, `about`, `discourse_username`). Synced from that level's YAML in the challenges repo (e.g. `docs/beginner.yaml`), which sets it only when someone other than the designer built the level. A value already in the website YAML always wins, so re-crediting a level upstream needs the same hand-edit as re-crediting the designer. **When omitted, the adventure designer is credited as the builder for this level.** When set, takes precedence over the adventure designer for credit display on the level page and in community leaderboard sections. See note below. | | `solved_count` | Optional | integer | Override for the displayed solved count. | | `top_players` | Optional | object[] | System-populated leaderboard data: `{username, count}`. Set by the leaderboard refresh script; do not edit by hand. | @@ -123,7 +123,7 @@ Go to **Actions → Sync Adventure from Challenges Repo → Run workflow**. The PR body lists everything that needs to happen before merging. Here is each item explained. -### Add contributor block +### Check the contributor block ```yaml contributor: @@ -133,7 +133,11 @@ contributor: discourse_username: "their_forum_username" ``` -Add this to `src/data/adventures//adventure.yaml`. The `url`, `about`, and `discourse_username` fields are optional but recommended -- `discourse_username` enables avatar resolution in community leaderboards. Once set, this block survives future re-syncs automatically. +This lives in `src/data/adventures//adventure.yaml`. The sync copies it from `contributor:` in the challenges repo's `docs/index.yaml` when the website YAML does not already have one, keeping only the four fields above -- the challenges repo owns its own schema, and passing an unknown field through would fail `npm run sync` against the strict content schema. Write it by hand only when the sync log warns that no contributor was found upstream. + +The `url`, `about`, and `discourse_username` fields are optional but recommended -- `discourse_username` enables avatar resolution in community leaderboards. A hand-edited block always wins over the upstream one and survives future re-syncs. + +An adventure with no designer cannot have levels that name their own builder: `creditIntegrityError` in `src/lib/adventure-credit.ts` fails `npm run sync` on that combination. ### Confirm month @@ -242,7 +246,8 @@ If the challenges repo is updated while your PR is still open, or you want to pr | Field | Preserved | Notes | | --- | --- | --- | -| `contributor:` (adventure) | Always | Survives every re-sync once set | +| `contributor:` (adventure) | Always | Survives every re-sync once set. When unset, the sync copies it from `docs/index.yaml` in the challenges repo | +| `contributor:` (level) | Always | Survives every re-sync once set, even if the level's YAML upstream names someone else. When unset, the sync copies it from that level's YAML in the challenges repo. Re-crediting a level is a deliberate hand-edit | | `community_category_id:` (adventure) | Always | Survives every re-sync once set; position is kept directly after `slug` | | `month:` (adventure) | Always | Survives every re-sync once set | | `discussion_url:` / `community_url:` (level) | Always | Website-only fields; never in the challenges repo. Both field aliases are preserved independently | diff --git a/scripts/sync-adventure.mjs b/scripts/sync-adventure.mjs index cc51aefb0..43d51b0be 100644 --- a/scripts/sync-adventure.mjs +++ b/scripts/sync-adventure.mjs @@ -152,6 +152,19 @@ function buildAdventureMetaDescription(indexData, activeLevels) { return truncateAtWord(full, 160); } +// The website's contributor schema is strict, so only the four fields it knows +// survive the trip from the challenges repo. Anything else in the upstream block +// would fail `astro sync` on a field the reviewer never wrote. +export function pickContributor(raw) { + if (!raw || typeof raw !== "object" || !raw.name) return null; + return { + name: raw.name, + ...(raw.url && { url: raw.url }), + ...(raw.about && { about: raw.about }), + ...(raw.discourse_username && { discourse_username: raw.discourse_username }), + }; +} + function transformStrings(value, fn) { if (typeof value === "string") return fn(value); if (Array.isArray(value)) return value.map((v) => transformStrings(v, fn)); @@ -268,14 +281,20 @@ function addToLucideIconsMap(content, iconName, kebab) { return content.replace(re, m[1] + block + m[3]); } -function buildLevel(raw, adventureTags, rewardsDeadline) { +export function buildLevel(raw, adventureTags, rewardsDeadline) { // architecture_diagram is stripped here. After all levels are fetched, the sync attempts // to pull the SVG from docs/diagrams/ in the challenges repo and re-adds the field if // successful. If not found there, it must be added manually to src/assets/diagrams/. const { architecture_diagram: _ignored, ...rest } = raw; - const cleaned = transformStrings(rest, stripCodeInLinks); + const { contributor: rawContributor, ...cleaned } = transformStrings(rest, stripCodeInLinks); + // A level `contributor` is the challenge builder, set upstream only when someone + // other than the adventure designer built this level. Filtered through the same + // picker as the designer: the website's contributor schema is strict, so a field + // the challenges repo carries but this site has no column for would fail sync. + const contributor = pickContributor(rawContributor); return { ...cleaned, + ...(contributor && { contributor }), ...(cleaned.deadline && { deadline: parseDeadline(cleaned.deadline, PRESERVE_TZ) }), topics: cleaned.topics || deriveTopics(adventureTags), verification: cleaned.verification || VERIFICATION_STUB, @@ -295,7 +314,7 @@ function buildLevel(raw, adventureTags, rewardsDeadline) { * injected by buildLevel, so manual edits are only preserved when * the upstream did not intentionally change the field. */ -function mergeLevels(existing, incoming, rawFetched) { +export function mergeLevels(existing, incoming, rawFetched) { const levelMap = Object.fromEntries((existing || []).map((l) => [l.level, l])); const rawMap = Object.fromEntries((rawFetched || []).map((l) => [l.level, l])); @@ -303,6 +322,18 @@ function mergeLevels(existing, incoming, rawFetched) { const prev = levelMap[l.level]; const raw = rawMap[l.level]; + // The challenge builder, already credited in the website YAML. Held aside + // because it wins over the upstream value rather than only filling a gap. + const preservedContributor = pickContributor(prev?.contributor); + const upstreamContributor = pickContributor(raw?.contributor); + if (preservedContributor && upstreamContributor && upstreamContributor.name !== preservedContributor.name) { + console.warn( + ` Level "${l.level}": keeping the builder already credited on the website ` + + `(${preservedContributor.name}) over the one named upstream (${upstreamContributor.name}). ` + + "Edit adventure.yaml by hand to re-credit this level." + ); + } + levelMap[l.level] = { ...l, // Preserve the discussion URL set by the add-discussion-url workflow. @@ -320,6 +351,12 @@ function mergeLevels(existing, incoming, rawFetched) { // intentional upstream changes come through. When the upstream did not set them (buildLevel // derived them from adventure tags), preserve any manual refinements from the website. ...(!raw?.topics && prev?.topics && { topics: prev.topics }), + // contributor: the challenge builder. A builder already credited on the website + // always wins, matching the adventure designer: the sync fills this field in from + // the level YAML, it never overwrites it. Re-crediting a level to someone else is + // a hand-edit either way, and losing an existing credit silently misattributes a + // person's work, which is worse than a stale credit a reviewer can see and fix. + ...(preservedContributor && { contributor: preservedContributor }), }; } @@ -471,6 +508,13 @@ async function main() { // Used for level flags and e2e/routes.ts generation below. const allLiveLevels = mergeLevels(existing?.levels, activeLevels, rawFetchedLevels); + const resolvedContributor = pickContributor(existing?.contributor) ?? pickContributor(indexData.contributor); + if (!existing?.contributor && resolvedContributor) { + console.log(` Contributor from docs/index.yaml: ${resolvedContributor.name}`); + } else if (!resolvedContributor) { + console.warn(" No contributor found in docs/index.yaml. Add a `contributor:` block to adventure.yaml before merging."); + } + // Build the combined adventure object using challenges repo field names. // The generator accepts all aliases (name/title, emoji → icon, etc.). const adventure = { @@ -494,8 +538,11 @@ async function main() { ...(indexData.rewards.deadline && { deadline: parseDeadline(indexData.rewards.deadline, PRESERVE_TZ) }), }, }), - // Preserve contributor set by a reviewer; omit otherwise (PR checklist item) - ...(existing?.contributor && { contributor: existing.contributor }), + // A reviewer's hand-edited contributor wins; otherwise take the designer the + // challenges repo already names in docs/index.yaml. Levels may credit their own + // builder, and the content schema rejects that on an adventure with no designer, + // so an unsynced upstream contributor fails the build rather than just losing a pill. + ...(resolvedContributor && { contributor: resolvedContributor }), ...(upcomingLevels.length > 0 && { upcoming_levels: upcomingLevels }), levels: allLiveLevels, }; diff --git a/src/content.config.ts b/src/content.config.ts index bb2cb8233..d11ba49e7 100644 --- a/src/content.config.ts +++ b/src/content.config.ts @@ -126,7 +126,8 @@ const levelSchema = z how_to_play: z.array(howToPlayStepSchema), verification: verificationSchema, helpful_links: z.array(helpfulLinkSchema).optional(), - meta_description: z.string().max(160).optional(), + // Length is warned about in renderLevel, not enforced: see warnIfMetaDescriptionLong. + meta_description: z.string().optional(), solved_count: z.number().int().optional(), top_players: z .array(z.object({ username: z.string(), count: z.number().int() }).strict()) @@ -144,6 +145,20 @@ const levelSchema = z // --- Resolvers --- +const META_DESCRIPTION_MAX = 160; + +// Over-length meta descriptions are an SEO smell, not a content error: search +// engines truncate the tail and the page still renders correctly. Warn so the +// sync PR gets refined before release, but never fail `astro sync` (and with it +// the whole sync-adventure workflow) over a description a reviewer can trim. +function warnIfMetaDescriptionLong(value: string | undefined, where: string): void { + if (value && value.length > META_DESCRIPTION_MAX) { + console.warn( + `[content] ${where}: meta_description is ${value.length} chars, over the ${META_DESCRIPTION_MAX}-char SEO limit. Search engines will truncate it.`, + ); + } +} + function requireEither(a: string | undefined | null, b: string | undefined | null, field: string): string { const value = a ?? b; if (value != null && value !== "") return value; @@ -187,7 +202,8 @@ function assertDifficulty( // AdventureLevel (from data/adventures/types.ts) is the single source of truth // for the rendered level shape. renderLevel's return type is checked against it, // so the two cannot drift silently. -async function renderLevel(level: z.infer): Promise { +async function renderLevel(level: z.infer, slug: string): Promise { + warnIfMetaDescriptionLong(level.meta_description, `${slug} → level "${level.level}"`); const difficulty = level.difficulty ?? (level.emoji ? LEVEL_DIFFICULTY_BY_EMOJI[level.emoji as keyof typeof LEVEL_DIFFICULTY_BY_EMOJI] : undefined); const learnings = level.learnings ?? level.what_you_learn ?? []; const intro = level.intro ?? (level.summary ? [level.summary] : undefined); @@ -345,7 +361,8 @@ const adventures = defineCollection({ tags: z.array(z.string()).min(1), contributor: contributorSchema.optional(), community_category_id: z.number().int().optional(), - meta_description: z.string().max(160), + // Length is warned about in the transform, not enforced: see warnIfMetaDescriptionLong. + meta_description: z.string(), backstory: z.array(z.string()).optional(), overview: z.array(z.string()).optional(), rewards: rewardsSchema.optional(), @@ -361,6 +378,7 @@ const adventures = defineCollection({ if (message) ctx.addIssue({ code: "custom", message, path: ["contributor"] }); }) .transform(async (data) => { + warnIfMetaDescriptionLong(data.meta_description, `adventure "${data.slug}"`); const title = requireEither(data.title, data.name, "adventure title/name"); const story = data.story ?? data.meta_description ?? (data.backstory && data.backstory.length > 0 ? data.backstory[0] : ""); @@ -370,7 +388,7 @@ const adventures = defineCollection({ mdToInline(story), data.contributor?.about ? mdToInline(data.contributor.about) : Promise.resolve(null), data.backstory ? mdToInlineArray(data.backstory) : Promise.resolve(null), - Promise.all(data.levels.map(renderLevel)), + Promise.all(data.levels.map((level) => renderLevel(level, data.slug))), data.rewards ? renderRewards(data.rewards) : Promise.resolve(null), ]); diff --git a/src/lib/adventure-credit.ts b/src/lib/adventure-credit.ts index 7f96afe74..c8785169c 100644 --- a/src/lib/adventure-credit.ts +++ b/src/lib/adventure-credit.ts @@ -103,8 +103,9 @@ export type PillCredit = { * An adventure with no designer has no level builders either, because the * content schema rejects that combination (see `creditIntegrityError`), so this * returns null rather than promoting a builder into the pill. The no-designer - * case is real: `sync-adventure.mjs` deliberately omits `contributor`, and a - * reviewer adds it as a PR checklist item. + * case is still real: `sync-adventure.mjs` copies `contributor` from the + * challenges repo's `docs/index.yaml`, so an adventure whose upstream names no + * designer yet arrives without one until a reviewer adds it. */ export function adventurePillCredit(adventure: CreditSource): PillCredit | null { const designer = adventure.contributor; diff --git a/src/test/lib/adventure-credit.test.ts b/src/test/lib/adventure-credit.test.ts index 32159e0f3..f3aa2c980 100644 --- a/src/test/lib/adventure-credit.test.ts +++ b/src/test/lib/adventure-credit.test.ts @@ -229,7 +229,8 @@ describe("creditIntegrityError", () => { expect(creditIntegrityError(adventure("a", KAT, [undefined, SIMON, undefined]))).toBeNull(); }); - // The freshly-synced state: sync-adventure.mjs omits `contributor` and a + // The freshly-synced state when the challenges repo names no designer in + // docs/index.yaml yet: sync-adventure.mjs emits no `contributor` and a // reviewer adds it later, so no-designer-and-no-builders must stay valid. it("passes an adventure with neither, which is what sync-adventure emits", () => { expect(creditIntegrityError(adventure("a", undefined))).toBeNull(); diff --git a/src/test/scripts/sync-adventure.test.ts b/src/test/scripts/sync-adventure.test.ts index 35b948c9b..a9b483158 100644 --- a/src/test/scripts/sync-adventure.test.ts +++ b/src/test/scripts/sync-adventure.test.ts @@ -16,7 +16,7 @@ */ import { describe, it, expect, vi, afterEach } from "vitest"; -import { currentMonth } from "../../../scripts/sync-adventure.mjs"; +import { buildLevel, currentMonth, mergeLevels, pickContributor } from "../../../scripts/sync-adventure.mjs"; const MONTH_SCHEMA = /^[A-Z]{3} \d{4}$/; @@ -54,3 +54,99 @@ describe("currentMonth", () => { expect(result).not.toMatch(MONTH_SCHEMA); }); }); + +describe("pickContributor", () => { + it("keeps the four fields the content schema accepts", () => { + expect( + pickContributor({ + name: "Ada Lovelace", + url: "https://example.com", + about: "Writes notes.", + discourse_username: "ada", + }), + ).toEqual({ + name: "Ada Lovelace", + url: "https://example.com", + about: "Writes notes.", + discourse_username: "ada", + }); + }); + + it("drops fields the strict content schema would reject", () => { + // The challenges repo owns its own index.yaml schema and may carry fields + // this site has no column for. Passing them through fails `astro sync`. + const result = pickContributor({ name: "Ada Lovelace", github: "ada", avatar: "a.png" }); + expect(result).toEqual({ name: "Ada Lovelace" }); + }); + + it("omits absent optional fields rather than emitting empty values", () => { + expect(pickContributor({ name: "Ada Lovelace", url: "", about: undefined })).toEqual({ + name: "Ada Lovelace", + }); + }); + + it.each([ + ["null", null], + ["undefined", undefined], + ["a block with no name", { url: "https://example.com" }], + ["a non-object", "Ada Lovelace"], + ])("returns null for %s", (_label, input) => { + expect(pickContributor(input)).toBeNull(); + }); +}); + +describe("buildLevel contributor (the challenge builder)", () => { + // The challenges repo puts the designer in docs/index.yaml and, only when a + // different person built a level, a `contributor:` in that level's YAML. When + // designer and builder are the same person the level YAML carries no + // contributor at all and the site falls back to the designer. + const base = { level: "beginner", topics: ["a11y"], verification: { command: "./v.sh", description: "d" } }; + + it("carries a level builder through from the level YAML", () => { + const result = buildLevel({ ...base, contributor: { name: "Grace Hopper", url: "https://example.com" } }, ["a11y"]); + expect(result.contributor).toEqual({ name: "Grace Hopper", url: "https://example.com" }); + }); + + it("drops builder fields the strict content schema would reject", () => { + const result = buildLevel({ ...base, contributor: { name: "Grace Hopper", github: "grace" } }, ["a11y"]); + expect(result.contributor).toEqual({ name: "Grace Hopper" }); + }); + + it("emits no contributor when the level YAML has none, so the designer is credited", () => { + const result = buildLevel({ ...base }, ["a11y"]); + expect(result).not.toHaveProperty("contributor"); + }); +}); + +describe("mergeLevels contributor preservation", () => { + const GRACE = { name: "Grace Hopper", url: "https://example.com" }; + const ADA = { name: "Ada Lovelace" }; + const lvl = (extra: object = {}) => ({ level: "beginner", topics: ["a11y"], ...extra }); + + it("keeps a builder already credited on the website when upstream names none", () => { + const merged = mergeLevels([lvl({ contributor: GRACE })], [lvl()], [lvl()]); + expect(merged[0].contributor).toEqual(GRACE); + }); + + it("keeps the website builder even when upstream names a different one", () => { + // Losing an existing credit silently misattributes someone's work. Re-crediting + // a level is a deliberate hand-edit, the same rule as the adventure designer. + const merged = mergeLevels([lvl({ contributor: GRACE })], [lvl({ contributor: ADA })], [lvl({ contributor: ADA })]); + expect(merged[0].contributor).toEqual(GRACE); + }); + + it("takes the upstream builder when the website has none", () => { + const merged = mergeLevels([lvl()], [lvl({ contributor: ADA })], [lvl({ contributor: ADA })]); + expect(merged[0].contributor).toEqual(ADA); + }); + + it("leaves a level with no builder on either side uncredited, so the designer is credited", () => { + const merged = mergeLevels([lvl()], [lvl()], [lvl()]); + expect(merged[0]).not.toHaveProperty("contributor"); + }); + + it("adds a builder to a level that is brand new to the website", () => { + const merged = mergeLevels([], [lvl({ contributor: ADA })], [lvl({ contributor: ADA })]); + expect(merged[0].contributor).toEqual(ADA); + }); +}); From 1df879a5b3b8ceaa188d9a74075e1f0aa10029af Mon Sep 17 00:00:00 2001 From: Sinduri Guntupalli Date: Mon, 7 Sep 2026 11:59:22 +0200 Subject: [PATCH 2/3] fix(sync): validate contributor data and register challenge tag routes Follow-up to the review of the previous commit, plus the route-coverage failure on PR #243. **Contributor data is now validated, not copied verbatim** - `contributor.url` is `z.url()` in the content schema, which rejects a bare domain like `ksick.dev`. An unvalidated copy failed `npm run sync` in the next workflow step, which runs before PR creation, so the run went red with no branch to hand-fix. The url is dropped with a warning and the name kept - `new URL()` is the gate, verified to match `z.url()` on every sample tested - A present-but-unusable block (no `name`, not an object) now warns instead of returning null in silence, which had reported "No contributor found" as fact and re-credited the level to the designer via the fallback - `pickContributor` takes a `where` label so each warning names its source file **Two failures moved to where they are actionable** - No designer upstream plus a level naming its own builder tripped `creditIntegrityError` during "Validate adventure YAML", before the PR existed. `missingDesignerError` now fails the sync early, naming `docs/index.yaml` in the challenges repo. A parity test holds it in step with the schema rule - A designer changed upstream was discarded with neither log branch firing; it now warns, matching the level builder **Challenge tag routes now come from the adventure tags** - `/challenges//` routes are built from adventure `tags`, but the sync derived them from level `topics`. Narrowing a level's topics to a subset, which the PR checklist invites, left those tags building unregistered routes - This is what failed route-coverage.spec.ts on #243 for `/challenges/accessibility/` and `/challenges/guidepup-virtual-screen-reader/` - `challengeTagsOf` unions adventure tags with level topics; `tagToSlug` has a parity test against `src/lib/challenges.ts` - Tests: 49 in the sync spec, including three parity guards against the TypeScript rules this script has to restate - Docs: ADVENTURES.md contributor url, early designer failure, routes step Signed-off-by: Sinduri Guntupalli --- ADVENTURES.md | 9 +- scripts/sync-adventure.mjs | 138 ++++++++++++++++--- src/test/scripts/sync-adventure.test.ts | 169 +++++++++++++++++++++++- 3 files changed, 287 insertions(+), 29 deletions(-) diff --git a/ADVENTURES.md b/ADVENTURES.md index a598bc821..7174463c6 100644 --- a/ADVENTURES.md +++ b/ADVENTURES.md @@ -114,8 +114,9 @@ Go to **Actions → Sync Adventure from Challenges Repo → Run workflow**. 2. If a PR branch (`feat/adventure-`) already exists, restores `adventure.yaml` from that branch so any manual edits already made survive the re-sync. 3. Fetches `docs/index.yaml` and all level YAMLs from the challenges repo. 4. Writes `src/data/adventures//adventure.yaml` and creates `-posts.json` stubs for each new live level. -5. Validates the YAML with `astro sync` (Zod content schema). There is no leaderboard registry to update: `buildAdventureCategories()` in `scripts/refresh-leaderboard.mjs` reads `community_category_id` out of every `adventure.yaml` at runtime, so setting that field (a PR checklist item below) is the whole registration step. Routes and sitemap entries are automatic via `getStaticPaths()` and `src/pages/sitemap.xml.ts`. `public/llms.txt` is updated by hand as part of the PR checklist. -6. Opens (or updates) a PR on `feat/adventure-` with a checklist of steps to complete before merging. +5. Registers the adventure's routes in `e2e/routes.ts` so the `route-coverage.spec.ts` drift gate passes without a manual edit. The `/challenges//` entries come from the adventure `tags`, which is what `getChallengeData()` in `src/lib/challenges.ts` builds the routes from, unioned with each level's `topics`. Deriving them from level topics alone under-reports: the sync seeds level topics from the adventure tags, but narrowing them to a level-specific subset (a PR checklist item) leaves any tag that appears on no level still building a route, and that route then fails the drift gate in CI. +6. Validates the YAML with `astro sync` (Zod content schema). There is no leaderboard registry to update: `buildAdventureCategories()` in `scripts/refresh-leaderboard.mjs` reads `community_category_id` out of every `adventure.yaml` at runtime, so setting that field (a PR checklist item below) is the whole registration step. Routes and sitemap entries are automatic via `getStaticPaths()` and `src/pages/sitemap.xml.ts`. `public/llms.txt` is updated by hand as part of the PR checklist. +7. Opens (or updates) a PR on `feat/adventure-` with a checklist of steps to complete before merging. --- @@ -135,6 +136,10 @@ contributor: This lives in `src/data/adventures//adventure.yaml`. The sync copies it from `contributor:` in the challenges repo's `docs/index.yaml` when the website YAML does not already have one, keeping only the four fields above -- the challenges repo owns its own schema, and passing an unknown field through would fail `npm run sync` against the strict content schema. Write it by hand only when the sync log warns that no contributor was found upstream. +`url` must be an absolute URL. A bare domain (`ksick.dev`) is rejected by the content schema, so the sync drops it, keeps the name, and warns. Add the `https://` scheme upstream to restore the link. + +If the challenges repo names no designer but a level YAML names a builder, the sync fails early with a message pointing at `docs/index.yaml`. Left to the content schema that failure lands in the next workflow step, before the PR branch exists, so there is nothing to hand-fix. + The `url`, `about`, and `discourse_username` fields are optional but recommended -- `discourse_username` enables avatar resolution in community leaderboards. A hand-edited block always wins over the upstream one and survives future re-syncs. An adventure with no designer cannot have levels that name their own builder: `creditIntegrityError` in `src/lib/adventure-credit.ts` fails `npm run sync` on that combination. diff --git a/scripts/sync-adventure.mjs b/scripts/sync-adventure.mjs index 43d51b0be..f3e3942d2 100644 --- a/scripts/sync-adventure.mjs +++ b/scripts/sync-adventure.mjs @@ -152,19 +152,104 @@ function buildAdventureMetaDescription(indexData, activeLevels) { return truncateAtWord(full, 160); } -// The website's contributor schema is strict, so only the four fields it knows -// survive the trip from the challenges repo. Anything else in the upstream block -// would fail `astro sync` on a field the reviewer never wrote. -export function pickContributor(raw) { - if (!raw || typeof raw !== "object" || !raw.name) return null; +// `contributor.url` is `z.url()` in the content schema, which rejects a bare +// domain like "ksick.dev". The challenges repo has its own schema and no reason +// to match ours, so an unvalidated copy fails `npm run sync` in the very next +// workflow step, before the PR branch exists for anyone to hand-fix. +// `new URL()` is the same gate `z.url()` applies. +function usableUrl(value, where) { + if (!value) return null; + try { + new URL(value); + return value; + } catch { + console.warn( + ` ${where}: contributor url "${value}" is not an absolute URL, which the website schema requires. ` + + "Dropped the link and kept the name. Add a scheme (https://) upstream to restore it." + ); + return null; + } +} + +/** + * Narrows an upstream contributor block to the four fields the website's strict + * contributor schema accepts. The challenges repo owns its own schema, so a + * field this site has no column for would otherwise fail `astro sync`. + * + * Absent returns null quietly. Present-but-unusable warns first: dropping it in + * silence re-credits the work to the adventure designer via the level fallback, + * which renders as a confident statement about who built something. + */ +export function pickContributor(raw, where = "contributor") { + if (raw == null) return null; + if (typeof raw !== "object" || Array.isArray(raw)) { + console.warn(` ${where}: contributor is ${Array.isArray(raw) ? "a list" : typeof raw}, expected a block with a \`name\`. Ignored.`); + return null; + } + if (!raw.name) { + console.warn(` ${where}: contributor block has no \`name\`, which the website schema requires. Ignored.`); + return null; + } + const url = usableUrl(raw.url, where); return { name: raw.name, - ...(raw.url && { url: raw.url }), + ...(url && { url }), ...(raw.about && { about: raw.about }), ...(raw.discourse_username && { discourse_username: raw.discourse_username }), }; } +// Mirrors `tagToSlug` in src/lib/challenges.ts, which builds the real route +// params. Held in step by a parity test; the script cannot import the TS module. +export const tagToSlug = (tag) => + tag.toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/(^-|-$)/g, ""); + +/** + * Every tag that generates a /challenges// route for this adventure. + * + * The routes come from the ADVENTURE `tags` (see `getChallengeData` in + * src/lib/challenges.ts, which flat-maps `a.tags`), not from level `topics`. + * Deriving them from level topics alone under-reports: the sync seeds each + * level's topics from the adventure tags, but the PR checklist invites reviewers + * to narrow them to a level-specific subset, and any adventure tag that then + * appears on no level still builds a route. That route reaches `dist/` + * unregistered and fails the route-coverage drift gate in CI. + * + * Level topics are still unioned in: a level may carry a topic of its own, and a + * surplus entry here is harmless because the gate Set-deduplicates. + */ +export function challengeTagsOf(adventureTags, levels) { + const fromLevels = (levels || []).flatMap((l) => + (l.topics ?? []).map((t) => (typeof t === "string" ? t : (t?.name ?? ""))), + ); + return [...new Set([...(adventureTags || []), ...fromLevels].filter(Boolean))]; +} + +/** + * Levels may only name their own builder on an adventure that names a designer. + * + * This is the same rule as `creditIntegrityError` in src/lib/adventure-credit.ts, + * which the content schema enforces. Checking it here too is not redundant: left + * to the schema it fails in the next workflow step ("Validate adventure YAML"), + * which runs before the PR is created, so the run goes red with no branch for + * anyone to hand-fix and an Astro error naming a file the reviewer cannot reach. + * Failing here names the upstream file that actually needs the edit. The two are + * held in step by a parity test, since this script is plain ESM and cannot import + * the TypeScript module. + * + * Returns the error message, or null when the adventure is valid. + */ +export function missingDesignerError(contributor, levels, indexPath) { + if (contributor) return null; + const named = (levels || []).filter((l) => l.contributor).map((l) => l.level); + if (named.length === 0) return null; + return ( + `Level(s) ${named.join(", ")} name their own \`contributor\` but the adventure has no designer.\n` + + "Every adventure needs a designer before its levels can credit separate builders.\n" + + `Add a \`contributor:\` block to ${indexPath}, then re-run this sync.` + ); +} + function transformStrings(value, fn) { if (typeof value === "string") return fn(value); if (Array.isArray(value)) return value.map((v) => transformStrings(v, fn)); @@ -291,7 +376,7 @@ export function buildLevel(raw, adventureTags, rewardsDeadline) { // other than the adventure designer built this level. Filtered through the same // picker as the designer: the website's contributor schema is strict, so a field // the challenges repo carries but this site has no column for would fail sync. - const contributor = pickContributor(rawContributor); + const contributor = pickContributor(rawContributor, `level "${raw.level}" upstream`); return { ...cleaned, ...(contributor && { contributor }), @@ -324,8 +409,8 @@ export function mergeLevels(existing, incoming, rawFetched) { // The challenge builder, already credited in the website YAML. Held aside // because it wins over the upstream value rather than only filling a gap. - const preservedContributor = pickContributor(prev?.contributor); - const upstreamContributor = pickContributor(raw?.contributor); + const preservedContributor = pickContributor(prev?.contributor, `level "${l.level}" in adventure.yaml`); + const upstreamContributor = pickContributor(raw?.contributor, `level "${l.level}" upstream`); if (preservedContributor && upstreamContributor && upstreamContributor.name !== preservedContributor.name) { console.warn( ` Level "${l.level}": keeping the builder already credited on the website ` + @@ -508,13 +593,30 @@ async function main() { // Used for level flags and e2e/routes.ts generation below. const allLiveLevels = mergeLevels(existing?.levels, activeLevels, rawFetchedLevels); - const resolvedContributor = pickContributor(existing?.contributor) ?? pickContributor(indexData.contributor); - if (!existing?.contributor && resolvedContributor) { - console.log(` Contributor from docs/index.yaml: ${resolvedContributor.name}`); + // The adventure designer. Same rule as the level builder: a designer already + // credited on the website wins, and the sync only fills the field in. + const websiteDesigner = pickContributor(existing?.contributor, "adventure.yaml"); + const upstreamDesigner = pickContributor(indexData.contributor, "docs/index.yaml"); + const resolvedContributor = websiteDesigner ?? upstreamDesigner; + + if (websiteDesigner && upstreamDesigner && websiteDesigner.name !== upstreamDesigner.name) { + console.warn( + ` Keeping the designer already credited in adventure.yaml (${websiteDesigner.name}) over the one named ` + + `in docs/index.yaml (${upstreamDesigner.name}). Edit adventure.yaml by hand to re-credit the adventure.` + ); + } else if (!websiteDesigner && upstreamDesigner) { + console.log(` Designer from docs/index.yaml: ${upstreamDesigner.name}`); } else if (!resolvedContributor) { - console.warn(" No contributor found in docs/index.yaml. Add a `contributor:` block to adventure.yaml before merging."); + console.warn(" No designer found in docs/index.yaml. Add a `contributor:` block there, or to adventure.yaml in the PR."); } + const designerError = missingDesignerError( + resolvedContributor, + allLiveLevels, + `${adventurePath}/docs/index.yaml in ${repo}`, + ); + if (designerError) fail(designerError); + // Build the combined adventure object using challenges repo field names. // The generator accepts all aliases (name/title, emoji → icon, etc.). const adventure = { @@ -605,15 +707,7 @@ async function main() { // ROUTES_WITHOUT_FULL_COVERAGE (or fully covered). We upsert a GENERATED block // so new tags are automatically registered; duplicates with the manual list or // other adventure blocks are harmless (Set-deduplicated at test time). - const tagToSlug = (tag) => - tag.toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/(^-|-$)/g, ""); - const allAdventureTags = [ - ...new Set( - allLiveLevels - .flatMap((l) => (l.topics ?? []).map((t) => (typeof t === "string" ? t : (t.name ?? "")))) - .filter(Boolean), - ), - ]; + const allAdventureTags = challengeTagsOf(adventureTags, allLiveLevels); if (allAdventureTags.length > 0) { const challengeBlockStart = ` // GENERATED:${slug}-challenges`; const challengeBlockEnd = ` // /GENERATED:${slug}-challenges`; diff --git a/src/test/scripts/sync-adventure.test.ts b/src/test/scripts/sync-adventure.test.ts index a9b483158..270083978 100644 --- a/src/test/scripts/sync-adventure.test.ts +++ b/src/test/scripts/sync-adventure.test.ts @@ -15,8 +15,19 @@ * schema, providing a second revert-catch that does not depend on the clock. */ -import { describe, it, expect, vi, afterEach } from "vitest"; -import { buildLevel, currentMonth, mergeLevels, pickContributor } from "../../../scripts/sync-adventure.mjs"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { z } from "zod"; +import { + buildLevel, + challengeTagsOf, + currentMonth, + mergeLevels, + missingDesignerError, + pickContributor, + tagToSlug, +} from "../../../scripts/sync-adventure.mjs"; +import { creditIntegrityError } from "@/lib/adventure-credit"; +import { tagToSlug as canonicalTagToSlug } from "@/lib/challenges"; const MONTH_SCHEMA = /^[A-Z]{3} \d{4}$/; @@ -56,6 +67,15 @@ describe("currentMonth", () => { }); describe("pickContributor", () => { + let warn: ReturnType; + + beforeEach(() => { + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + afterEach(() => { + warn.mockRestore(); + }); + it("keeps the four fields the content schema accepts", () => { expect( pickContributor({ @@ -70,6 +90,7 @@ describe("pickContributor", () => { about: "Writes notes.", discourse_username: "ada", }); + expect(warn).not.toHaveBeenCalled(); }); it("drops fields the strict content schema would reject", () => { @@ -88,10 +109,55 @@ describe("pickContributor", () => { it.each([ ["null", null], ["undefined", undefined], + ])("returns null for %s without warning", (_label, input) => { + expect(pickContributor(input)).toBeNull(); + expect(warn).not.toHaveBeenCalled(); + }); + + it.each([ ["a block with no name", { url: "https://example.com" }], ["a non-object", "Ada Lovelace"], - ])("returns null for %s", (_label, input) => { - expect(pickContributor(input)).toBeNull(); + ["a list", [{ name: "Ada Lovelace" }]], + ])("returns null for %s and says so", (_label, input) => { + // Silence here would re-credit the level to the designer via the fallback, + // and print "no contributor found", both stated as fact. + expect(pickContributor(input, "docs/index.yaml")).toBeNull(); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain("docs/index.yaml"); + }); + + describe("url validation", () => { + // `contributor.url` is `z.url()` in the content schema. An unusable value + // would fail `npm run sync` in the next workflow step, before a PR branch + // exists to hand-fix, so it is dropped here with the name kept. + it.each([ + ["a bare domain", "ksick.dev"], + ["a www host with no scheme", "www.example.com"], + ["a relative path", "/about"], + ])("drops %s and keeps the name", (_label, url) => { + expect(pickContributor({ name: "Ada Lovelace", url })).toEqual({ name: "Ada Lovelace" }); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain(url); + }); + + it.each([ + ["https", "https://example.com"], + ["http", "http://example.com"], + ["a path and query", "https://example.com/a?b=1#c"], + ])("keeps a valid %s url", (_label, url) => { + expect(pickContributor({ name: "Ada Lovelace", url })).toEqual({ name: "Ada Lovelace", url }); + expect(warn).not.toHaveBeenCalled(); + }); + + it("agrees with the z.url() gate the content schema applies", () => { + // Parity check: if Zod's rule and this one drift, the sync starts emitting + // YAML that fails validation again, which is the bug this guards. + const schema = z.url(); + for (const url of ["https://a.dev", "ksick.dev", "http://a.b", "www.x.com", "mailto:a@b.c", "/rel"]) { + const kept = pickContributor({ name: "N", url })?.url !== undefined; + expect(kept).toBe(schema.safeParse(url).success); + } + }); }); }); @@ -118,6 +184,94 @@ describe("buildLevel contributor (the challenge builder)", () => { }); }); +describe("challengeTagsOf", () => { + it("includes an adventure tag that no level carries as a topic", () => { + // PR #243 failed exactly here. "Accessibility" and "Guidepup Virtual Screen + // Reader" were adventure tags refined out of every level's topics, so the old + // level-topics-only derivation missed them, their /challenges// routes + // reached dist/ unregistered, and route-coverage.spec.ts failed. + const tags = challengeTagsOf( + ["Accessibility", "Guidepup Virtual Screen Reader", "Playwright"], + [{ level: "beginner", topics: ["Playwright"] }], + ); + expect(tags).toContain("Accessibility"); + expect(tags).toContain("Guidepup Virtual Screen Reader"); + }); + + it("unions level topics that are not adventure tags", () => { + expect(challengeTagsOf(["A"], [{ level: "l", topics: ["B"] }])).toEqual(["A", "B"]); + }); + + it("deduplicates a tag present on both the adventure and a level", () => { + expect(challengeTagsOf(["A"], [{ level: "l", topics: ["A"] }])).toEqual(["A"]); + }); + + it("accepts object-shaped topics and drops empty entries", () => { + expect(challengeTagsOf([], [{ level: "l", topics: [{ name: "A" }, { name: "" }, ""] }])).toEqual(["A"]); + }); + + it.each([ + ["no levels", ["A"], []], + ["levels with no topics", ["A"], [{ level: "l" }]], + ])("still reports adventure tags with %s", (_label, adventureTags, levels) => { + expect(challengeTagsOf(adventureTags, levels)).toEqual(["A"]); + }); + + it("slugs tags the same way the route params are built", () => { + // Drift against src/lib/challenges.ts would register a route under a slug the + // build never emits, leaving the real one unregistered and CI red. + for (const tag of ["Accessibility", "Guidepup Virtual Screen Reader", "ArgoCD", "C++", " Trim Me "]) { + expect(tagToSlug(tag)).toBe(canonicalTagToSlug(tag)); + } + }); +}); + +describe("missingDesignerError", () => { + const lvls = (...contributors: (object | undefined)[]) => + contributors.map((c, i) => ({ level: `l${i}`, ...(c && { contributor: c }) })); + + it("fires when a level names a builder but the adventure has no designer", () => { + const msg = missingDesignerError(undefined, lvls({ name: "Ada" }, undefined), "docs/index.yaml in some/repo"); + expect(msg).toContain("l0"); + expect(msg).toContain("docs/index.yaml in some/repo"); + }); + + it("names every offending level, not just the first", () => { + const msg = missingDesignerError(undefined, lvls({ name: "Ada" }, undefined, { name: "Grace" }), "p"); + expect(msg).toContain("l0, l2"); + }); + + it.each([ + ["a designer is set", { name: "Ada" }, lvls({ name: "Grace" })], + ["no level names a builder", undefined, lvls(undefined, undefined)], + ["there are no levels", undefined, []], + ])("passes when %s", (_label, contributor, levels) => { + expect(missingDesignerError(contributor, levels, "p")).toBeNull(); + }); + + it("agrees with creditIntegrityError, the rule the content schema enforces", () => { + // The script cannot import the TypeScript module, so the rule is stated twice. + // Drift means the sync writes YAML that then fails validation, which is exactly + // the failure this gate exists to pre-empt. + const cases: { contributor?: { name: string }; levels: { contributor?: unknown }[] }[] = [ + { contributor: undefined, levels: lvls({ name: "Ada" }) }, + { contributor: undefined, levels: lvls(undefined) }, + { contributor: { name: "Ada" }, levels: lvls({ name: "Grace" }) }, + { contributor: { name: "Ada" }, levels: lvls(undefined) }, + { contributor: undefined, levels: [] }, + ]; + let fired = 0; + for (const { contributor, levels } of cases) { + const scriptFires = missingDesignerError(contributor, levels, "p") !== null; + const schemaFires = creditIntegrityError({ slug: "s", contributor, levels }) !== null; + expect(scriptFires).toBe(schemaFires); + if (scriptFires) fired++; + } + // Both agreeing on "never fires" would satisfy the loop without testing anything. + expect(fired).toBeGreaterThan(0); + }); +}); + describe("mergeLevels contributor preservation", () => { const GRACE = { name: "Grace Hopper", url: "https://example.com" }; const ADA = { name: "Ada Lovelace" }; @@ -128,11 +282,16 @@ describe("mergeLevels contributor preservation", () => { expect(merged[0].contributor).toEqual(GRACE); }); - it("keeps the website builder even when upstream names a different one", () => { + it("keeps the website builder even when upstream names a different one, and says so", () => { // Losing an existing credit silently misattributes someone's work. Re-crediting // a level is a deliberate hand-edit, the same rule as the adventure designer. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const merged = mergeLevels([lvl({ contributor: GRACE })], [lvl({ contributor: ADA })], [lvl({ contributor: ADA })]); expect(merged[0].contributor).toEqual(GRACE); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain("Grace Hopper"); + expect(warn.mock.calls[0][0]).toContain("Ada Lovelace"); + warn.mockRestore(); }); it("takes the upstream builder when the website has none", () => { From 68ed62e54f232a7066832001d50569e96affde7b Mon Sep 17 00:00:00 2001 From: Sinduri Guntupalli Date: Mon, 7 Sep 2026 12:29:10 +0200 Subject: [PATCH 3/3] fix(challenges): match tag pages on level topics, not adventure tags A /challenges// page listed every challenge in any adventure carrying that tag, including challenges that never touch the tech. `getChallengeData` gave each entry `adventureTags: a.tags`, and `level.topics` was declared on the type but never read. **Three routes were affected, two of them live** - `/challenges/guidepup-virtual-screen-reader/` (PR #243) showed the beginner challenge; Guidepup belongs to the intermediate level, which is not live yet - `/challenges/python/` and `/challenges/terraform/` had the same defect already **The fix** - `ChallengeEntry.adventureTags` becomes `topics`, taken from the level, falling back to adventure tags only for a level carrying none - The tag set is derived from the entries, so a tag exists exactly when some challenge teaches it. A tag no live level carries builds no route rather than an empty or misleading one, and appears when that level ships - `challengeTagsOf` in the sync now mirrors this, deriving from level topics. Registering an adventure tag would list a route the build never emits and trip the drift gate from the other side **Data** - Python added to all three the-ai-observatory level topics. It is genuinely taught there (6-8 mentions per level) and was simply missing, so the route stays and is now correct - Terraform removed from building-cloudhaven tags. That adventure teaches OpenTofu; "Terraform" appeared once, inside an OpenTofu help link. No redirect, so the slug stays free for a future adventure that does teach Terraform - `/challenges/terraform/` dropped from ROUTES_WITHOUT_FULL_COVERAGE Verified against the built output: /challenges/prometheus/ now shows 1 challenge rather than all three ai-observatory levels, and every visible card carries the filtered tag. Signed-off-by: Sinduri Guntupalli --- ADVENTURES.md | 6 ++-- e2e/routes.ts | 3 +- scripts/sync-adventure.mjs | 26 +++++++------- src/components/ChallengesFilter.astro | 4 +-- .../building-cloudhaven/adventure.yaml | 1 - .../the-ai-observatory/adventure.yaml | 3 ++ src/lib/challenges.ts | 22 +++++++++--- src/test/lib/challenges.test.ts | 23 ++++++++++--- src/test/scripts/sync-adventure.test.ts | 34 +++++++++++-------- 9 files changed, 79 insertions(+), 43 deletions(-) diff --git a/ADVENTURES.md b/ADVENTURES.md index 7174463c6..6c865e278 100644 --- a/ADVENTURES.md +++ b/ADVENTURES.md @@ -39,7 +39,7 @@ The authoritative schema is in [`src/content.config.ts`](src/content.config.ts) | `icon` | Optional | Lucide icon name (e.g. `Satellite`) | The sync workflow auto-registers the icon (imports, type union, emoji mapping) and writes `icon:` directly into `adventure.yaml`. Set explicitly in the challenges repo when the emoji alone is insufficient. | | `emoji` | Optional | emoji character | Shown on the adventure card. The sync workflow maps it to a Lucide icon via the `EMOJI_TO_ICON` table; add the mapping there first if the emoji is new. | | `month` | **Required** | `MMM YYYY` | Three-letter uppercase abbreviation + four-digit year. Allowed: `JAN FEB MAR APR MAY JUN JUL AUG SEP OCT NOV DEC`. Validated by Zod regex; wrong format fails sync. | -| `tags` | **Required** | `string[]` (min 1) | Technology/topic labels shown as filter chips. Used when the auto-generated `meta_description` falls back to name + backstory. | +| `tags` | **Required** | `string[]` (min 1) | Technology/topic labels for the adventure as a whole. Seeds each level's `topics` at sync time and feeds the auto-generated `meta_description` fallback. **Does not by itself create a filter chip or a `/challenges//` route:** those come from level `topics`, so a tag no level teaches builds nothing. Add the tag to the relevant level's `topics` to make it filterable. | | `meta_description` | **Required** | string, 160 chars recommended | Validated by the Zod schema; missing field fails `npm run sync`. Over 160 chars logs a warning and still builds, since search engines truncate rather than reject. Trim it before release. No em dashes; no ` - ` used as a dash. | | `story` | Optional | markdown string | Short description shown on adventure cards and at the top of the adventure page. Card views strip HTML; set:html prose uses the rendered version. | | `backstory` | Optional | `string[]` (markdown) | Thematic narrative paragraphs rendered on the adventure page. | @@ -61,7 +61,7 @@ Each entry in the `levels` array accepts the following fields. | `level` | **Required** | string | Level identifier and URL segment: `beginner`, `intermediate`, or `expert`. | | `name` or `title` | **One required** | string | Display name for the level. | | `devcontainer` | **Required** | string | Devcontainer folder name in the challenges repo `.devcontainer/` directory. | -| `topics` | **Required** | `string[]` | Technologies covered by this level. An empty `[]` is valid and stays empty; inheriting adventure `tags` is done by the sync workflow, not the schema. | +| `topics` | **Required** | `string[]` | Technologies covered by this level, and **the source of truth for the `/challenges/` filter chips and `/challenges//` routes**. A tag page lists exactly the levels carrying that topic, so narrowing this list narrows what the level appears under. An empty `[]` is valid and falls back to the adventure `tags` for filtering; the sync seeds it from them, and inheriting is done by the sync workflow, not the schema. | | `objective` | **Required** | `string[]` (markdown) | Success criteria list shown to participants. | | `toolbox` | **Required** | object[] | `{name, description, url?}` — tools available in the level environment. | | `how_to_play` | **Required** | object[] | `{id?, title, content}` — ordered step-by-step instructions. | @@ -114,7 +114,7 @@ Go to **Actions → Sync Adventure from Challenges Repo → Run workflow**. 2. If a PR branch (`feat/adventure-`) already exists, restores `adventure.yaml` from that branch so any manual edits already made survive the re-sync. 3. Fetches `docs/index.yaml` and all level YAMLs from the challenges repo. 4. Writes `src/data/adventures//adventure.yaml` and creates `-posts.json` stubs for each new live level. -5. Registers the adventure's routes in `e2e/routes.ts` so the `route-coverage.spec.ts` drift gate passes without a manual edit. The `/challenges//` entries come from the adventure `tags`, which is what `getChallengeData()` in `src/lib/challenges.ts` builds the routes from, unioned with each level's `topics`. Deriving them from level topics alone under-reports: the sync seeds level topics from the adventure tags, but narrowing them to a level-specific subset (a PR checklist item) leaves any tag that appears on no level still building a route, and that route then fails the drift gate in CI. +5. Registers the adventure's routes in `e2e/routes.ts` so the `route-coverage.spec.ts` drift gate passes without a manual edit. The `/challenges//` entries are derived from each live level's `topics`, mirroring `getChallengeData()` in `src/lib/challenges.ts`. Adventure `tags` are deliberately not used: a tag no level teaches builds no route, so registering one would list a route that never reaches `dist/` and trip the drift gate from the other side. 6. Validates the YAML with `astro sync` (Zod content schema). There is no leaderboard registry to update: `buildAdventureCategories()` in `scripts/refresh-leaderboard.mjs` reads `community_category_id` out of every `adventure.yaml` at runtime, so setting that field (a PR checklist item below) is the whole registration step. Routes and sitemap entries are automatic via `getStaticPaths()` and `src/pages/sitemap.xml.ts`. `public/llms.txt` is updated by hand as part of the PR checklist. 7. Opens (or updates) a PR on `feat/adventure-` with a checklist of steps to complete before merging. diff --git a/e2e/routes.ts b/e2e/routes.ts index e8d2e4014..afb0129b1 100644 --- a/e2e/routes.ts +++ b/e2e/routes.ts @@ -94,7 +94,7 @@ export const A11Y_PAGES: string[] = [ * gate in route-coverage.spec.ts. */ export const ROUTES_WITHOUT_FULL_COVERAGE: string[] = [ - // 24 remaining /challenges// routes. They are the same page component with + // 23 remaining /challenges// routes. They are the same page component with // a different filter seed, so /challenges/opentelemetry/ is the representative. // Listed explicitly rather than pattern-matched so a new tag still has to be // acknowledged here. @@ -120,6 +120,5 @@ export const ROUTES_WITHOUT_FULL_COVERAGE: string[] = [ "/challenges/python/", "/challenges/spring-boot/", "/challenges/tdd/", - "/challenges/terraform/", "/challenges/trivy/", ]; diff --git a/scripts/sync-adventure.mjs b/scripts/sync-adventure.mjs index f3e3942d2..d0f1521d4 100644 --- a/scripts/sync-adventure.mjs +++ b/scripts/sync-adventure.mjs @@ -207,22 +207,22 @@ export const tagToSlug = (tag) => /** * Every tag that generates a /challenges// route for this adventure. * - * The routes come from the ADVENTURE `tags` (see `getChallengeData` in - * src/lib/challenges.ts, which flat-maps `a.tags`), not from level `topics`. - * Deriving them from level topics alone under-reports: the sync seeds each - * level's topics from the adventure tags, but the PR checklist invites reviewers - * to narrow them to a level-specific subset, and any adventure tag that then - * appears on no level still builds a route. That route reaches `dist/` - * unregistered and fails the route-coverage drift gate in CI. + * Mirrors `getChallengeData` in src/lib/challenges.ts, which derives the tag set + * from each live level's `topics`. Adventure `tags` are deliberately NOT included: + * a tag no level teaches builds no route there, so registering it here would list + * a route that never reaches `dist/` and trip the drift gate from the other side. * - * Level topics are still unioned in: a level may carry a topic of its own, and a - * surplus entry here is harmless because the gate Set-deduplicates. + * The `adventureTags` fallback matches the same fallback in `getChallengeData`, + * which applies only to a level carrying no topics of its own. */ export function challengeTagsOf(adventureTags, levels) { - const fromLevels = (levels || []).flatMap((l) => - (l.topics ?? []).map((t) => (typeof t === "string" ? t : (t?.name ?? ""))), - ); - return [...new Set([...(adventureTags || []), ...fromLevels].filter(Boolean))]; + const perLevel = (levels || []).map((l) => { + const topics = (l.topics ?? []) + .map((t) => (typeof t === "string" ? t : (t?.name ?? ""))) + .filter(Boolean); + return topics.length > 0 ? topics : (adventureTags || []); + }); + return [...new Set(perLevel.flat().filter(Boolean))]; } /** diff --git a/src/components/ChallengesFilter.astro b/src/components/ChallengesFilter.astro index 9bd1e7506..488b754ff 100644 --- a/src/components/ChallengesFilter.astro +++ b/src/components/ChallengesFilter.astro @@ -35,7 +35,7 @@ const { entries, tags, base, initialTag, adventureCount, embedded, seeAllHref } const activeTags = initialTag ? [initialTag] : []; const hasFilters = activeTags.length > 0; const matches = (e: ChallengeEntry): boolean => - activeTags.length === 0 || activeTags.some((t) => e.adventureTags.includes(t)); + activeTags.length === 0 || activeTags.some((t) => e.topics.includes(t)); const matchedCount = entries.filter(matches).length; const countText = hasFilters @@ -253,7 +253,7 @@ const dropdownItem = class="contents" data-level-card data-difficulty={e.difficulty} - data-tags={e.adventureTags.map(tagToSlug).join(" ")} + data-tags={e.topics.map(tagToSlug).join(" ")} hidden={!matches(e)} > RAG pipelines with OpenLLMetry - Create custom OpenTelemetry metrics in Python @@ -221,6 +223,7 @@ levels: - OpenTelemetry - OpenLLMetry - Jaeger + - Python learnings: - OpenTelemetry GenAI semantic conventions - Tail sampling in the OTel Collector diff --git a/src/lib/challenges.ts b/src/lib/challenges.ts index bcccb1764..d178d8737 100644 --- a/src/lib/challenges.ts +++ b/src/lib/challenges.ts @@ -1,6 +1,6 @@ // Derives the flat, filterable level list and the tag set for /challenges, // from the adventures collection. -// tags are ADVENTURE tags (OR match); levels are then filtered by difficulty. +// Tags are per-LEVEL topics (OR match); levels are then filtered by difficulty. import { type Difficulty } from "@/lib/difficulty"; @@ -89,7 +89,14 @@ export type ChallengeEntry = { estimatedTime?: string; adventureId: string; adventureTitle: string; - adventureTags: string[]; + /** + * What this specific challenge teaches, and what a /challenges// page + * matches on. Named for the level, not the adventure, because they differ: an + * adventure tag can belong to one level (or to a level that is not live yet), + * and matching on the adventure's tags listed every challenge in it, including + * ones that never touch the tech. + */ + topics: string[]; adventureIcon?: string; isLive: boolean; url: string; @@ -126,12 +133,19 @@ export function getChallengeData(adventures: AdventureData[]): { estimatedTime: level.estimatedTime, adventureId: a.slug, adventureTitle: a.title, - adventureTags: a.tags, + // Fall back to the adventure tags only when a level carries no topics of + // its own, which the sync never produces (it seeds them from the adventure + // tags). Without the guard a topic-less level would drop out of every tag + // page instead of degrading to the old, broader behaviour. + topics: level.topics.length > 0 ? level.topics : a.tags, adventureIcon: a.icon, isLive: live, url: `/adventures/${a.slug}/levels/${level.id}/`, })); }); - const tags = Array.from(new Set(adventures.flatMap((a) => a.tags))).sort(); + // Derived from the entries, so a tag exists exactly when some challenge + // teaches it. An adventure tag that no live level carries builds no route + // rather than an empty or misleading one; it appears when that level ships. + const tags = Array.from(new Set(entries.flatMap((e) => e.topics))).sort(); return { entries, tags }; } diff --git a/src/test/lib/challenges.test.ts b/src/test/lib/challenges.test.ts index c8ece1503..bd92da6bf 100644 --- a/src/test/lib/challenges.test.ts +++ b/src/test/lib/challenges.test.ts @@ -271,11 +271,26 @@ describe("getChallengeData", () => { expect(entries[0].url).toBe("/adventures/my-slug/levels/level-1/"); }); - it("includes the adventureTags array on the entry", () => { + it("takes the entry topics from the level, not the adventure", () => { + // A tag page matches on this, so an adventure tag the level does not teach + // must not appear here: that listed challenges that never touch the tech. const { entries } = getChallengeData([ - makeAdventure({ tags: ["rust", "cloud"] }), + makeAdventure({ + tags: ["rust", "cloud"], + levels: [{ id: "l1", name: "L1", difficulty: "Beginner", topics: ["rust"] }], + }), + ]); + expect(entries[0].topics).toEqual(["rust"]); + }); + + it("falls back to the adventure tags when a level carries no topics", () => { + const { entries } = getChallengeData([ + makeAdventure({ + tags: ["rust", "cloud"], + levels: [{ id: "l1", name: "L1", difficulty: "Beginner", topics: [] }], + }), ]); - expect(entries[0].adventureTags).toEqual(["rust", "cloud"]); + expect(entries[0].topics).toEqual(["rust", "cloud"]); }); it("copies the icon from the adventure", () => { @@ -414,7 +429,7 @@ describe("getChallengeData", () => { expect(entry).toHaveProperty("learnings"); expect(entry).toHaveProperty("adventureId"); expect(entry).toHaveProperty("adventureTitle"); - expect(entry).toHaveProperty("adventureTags"); + expect(entry).toHaveProperty("topics"); expect(entry).toHaveProperty("isLive"); expect(entry).toHaveProperty("url"); }); diff --git a/src/test/scripts/sync-adventure.test.ts b/src/test/scripts/sync-adventure.test.ts index 270083978..b71c195ee 100644 --- a/src/test/scripts/sync-adventure.test.ts +++ b/src/test/scripts/sync-adventure.test.ts @@ -185,25 +185,25 @@ describe("buildLevel contributor (the challenge builder)", () => { }); describe("challengeTagsOf", () => { - it("includes an adventure tag that no level carries as a topic", () => { - // PR #243 failed exactly here. "Accessibility" and "Guidepup Virtual Screen - // Reader" were adventure tags refined out of every level's topics, so the old - // level-topics-only derivation missed them, their /challenges// routes - // reached dist/ unregistered, and route-coverage.spec.ts failed. + it("omits an adventure tag that no level teaches", () => { + // The #243 case. "Guidepup Virtual Screen Reader" belongs to an intermediate + // level that is not live yet, so it must build no route: registering one for + // a route the build never emits trips the drift gate from the other side. const tags = challengeTagsOf( ["Accessibility", "Guidepup Virtual Screen Reader", "Playwright"], [{ level: "beginner", topics: ["Playwright"] }], ); - expect(tags).toContain("Accessibility"); - expect(tags).toContain("Guidepup Virtual Screen Reader"); + expect(tags).toEqual(["Playwright"]); }); - it("unions level topics that are not adventure tags", () => { - expect(challengeTagsOf(["A"], [{ level: "l", topics: ["B"] }])).toEqual(["A", "B"]); + it("reports the union of the topics levels do teach", () => { + expect( + challengeTagsOf(["A"], [{ level: "l1", topics: ["B"] }, { level: "l2", topics: ["C"] }]), + ).toEqual(["B", "C"]); }); - it("deduplicates a tag present on both the adventure and a level", () => { - expect(challengeTagsOf(["A"], [{ level: "l", topics: ["A"] }])).toEqual(["A"]); + it("deduplicates a topic two levels share", () => { + expect(challengeTagsOf([], [{ level: "l1", topics: ["A"] }, { level: "l2", topics: ["A"] }])).toEqual(["A"]); }); it("accepts object-shaped topics and drops empty entries", () => { @@ -211,12 +211,18 @@ describe("challengeTagsOf", () => { }); it.each([ - ["no levels", ["A"], []], - ["levels with no topics", ["A"], [{ level: "l" }]], - ])("still reports adventure tags with %s", (_label, adventureTags, levels) => { + ["a level carries no topics", ["A"], [{ level: "l" }]], + ["a level has an empty topics list", ["A"], [{ level: "l", topics: [] }]], + ])("falls back to adventure tags when %s", (_label, adventureTags, levels) => { + // Same fallback as getChallengeData, so a topic-less level degrades to the + // old broader behaviour instead of dropping out of every tag page. expect(challengeTagsOf(adventureTags, levels)).toEqual(["A"]); }); + it("reports nothing when there are no levels", () => { + expect(challengeTagsOf(["A"], [])).toEqual([]); + }); + it("slugs tags the same way the route params are built", () => { // Drift against src/lib/challenges.ts would register a route under a slug the // build never emits, leaving the real one unregistered and CI red.