fix(web): enforce catalog installability lifecycle - #312
Conversation
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| export async function loadSkillsIndex(fetchImpl, { signal } = {}) { | ||
| if (typeof fetchImpl !== "function") { | ||
| throw new Error("A fetch implementation is required"); | ||
| } | ||
|
|
||
| const response = await fetchImpl(SKILLS_INDEX_PATH, { | ||
| headers: { Accept: "application/json" }, | ||
| signal, | ||
| }); | ||
| if (!response?.ok) { | ||
| const status = Number.isInteger(response?.status) ? ` (HTTP ${response.status})` : ""; | ||
| throw new Error(`Skills index is unavailable${status}`); |
There was a problem hiding this comment.
Catalog loaders duplicate fetch parsing
loadSkillsIndex() repeats the same fetch/response flow that fetchRequiredSkillData() and the optional fetchers inline, so headers or JSON/HTML fallback changes have to be updated in four places — should we factor a small response-reader helper and keep the catalog-specific validation in the callers?
Want Baz to fix this for you? Activate Fixer
| validateOptionalInstallable(skill, "Skill metadata"); | ||
| if (hasOwn(skill, "tag")) { | ||
| requireNonEmptyString(skill.tag, "tag", "Skill metadata"); | ||
| } | ||
| return skill; |
There was a problem hiding this comment.
Validator allows missing Skill fields
parseSkillData() only checks name, version, installable, and tag before returning skill, so loadSkillDetailData() can pass pages/SkillDetail.tsx a skillData that misses required SkillJson fields like description and author, and the page then renders undefined/blank values instead of a blocked state — should we validate the full contract before returning?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
utils/skillCatalogInstallability.mjs around lines 201-205, inside the parseSkillData()
logic, the function currently validates only name/version/(optional
installable)/(optional non-empty tag) and then immediately returns skill without
enforcing the rest of the required SkillJson fields. Update parseSkillData() to validate
every required property in the shared SkillJson contract (e.g., description, author,
license, homepage, keywords, sbom), using the existing
requireNonEmptyString/validateOptionalInstallable helpers (and add small helpers if
needed for array/object shapes) and throw an Error when any required field is missing or
invalid. This will ensure loadSkillDetailData() catches the error and returns a blocked
lifecycle state instead of returning invalid skillData that pages/SkillDetail.tsx
renders as undefined/blank.
| function assertBlocked(result) { | ||
| assertExactResultShape(result); | ||
| assert.equal(result.state, "blocked"); | ||
| assert.deepEqual(result.view, BLOCKED_VIEW); | ||
| assert.equal(result.view.canInstall, false); | ||
| assert.equal(result.view.showCopyControls, false); | ||
| assert.equal(result.view.showPlatforms, false); | ||
| assert.equal(result.view.showTriggers, false); | ||
| assert.equal(result.view.showDocumentation, false); | ||
| } |
There was a problem hiding this comment.
Redundant blocked-state assertions
assertBlocked() already checks result.view against BLOCKED_VIEW with assert.deepEqual, so the four assert.equal(...) calls just repeat the same contract and add noise to every blocked-path test — should we keep the single deep-equality assertion and drop the per-field checks?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
scripts/test-web-catalog-installability.mjs around lines 127-136, the
assertBlocked(result) helper repeats redundant assertions by checking individual view
properties (lines 131-136) after already doing assert.deepEqual(result.view,
BLOCKED_VIEW). Remove the four per-field assert.equal(...) checks and keep only
assertExactResultShape(result) plus the single deep-equality assertion of result.view
against BLOCKED_VIEW. This preserves the same coverage while reducing noise in all
blocked-path tests that use assertBlocked().
| const repoBase = `${url.origin}/${owner}/${repo.replace(/\\.git$/, '')}`; | ||
| releasePageUrl = `${repoBase}/releases/tag/${encodeURIComponent(releaseTag)}`; |
There was a problem hiding this comment.
Broken GitHub release links for .git
repo.replace(/\.git$/, '') only matches a backslash-escaped .git, so repo.git keeps the suffix and the release link points at .../repo.git/releases/tag/<tag> instead of GitHub's /owner/repo/releases/tag/<tag> — should we change it to /.git$/?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
pages/SkillDetail.tsx around lines 234-247, inside the `try`/`catch` logic that builds
`releasePageUrl` from `skillData.homepage` for GitHub repos, fix the `.git` suffix
stripping: the code currently does `repo.replace(/\.git$/, '')`, which incorrectly looks
for a literal backslash before `git` and fails to remove a real `repo.git` suffix.
Refactor this single replace to use the correct suffix regex `/.git$/` so `repo.git`
becomes `repo` and the generated URL becomes `.../owner/repo/releases/tag/<tag>`. Keep
the existing fallback behavior in the `catch` so invalid URLs still return the canonical
`RELEASE_REPO_URL` link.
| {view.canInstall && view.showCopyControls && ( | ||
| <section className="space-y-4"> | ||
| <h2 className="text-xl font-bold text-white flex items-center gap-2"> | ||
| <Download size={20} /> |
There was a problem hiding this comment.
Redundant gate bloats install section
view.canInstall is already ruled out by the earlier return, so the extra {view.canInstall && ...} here just adds a branch without changing the rendered output — should we drop it and keep only {view.showCopyControls && (...)}?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
pages/SkillDetail.tsx around lines 308-335, in the JSX for the “Install Command /
Quick Install” section, remove the redundant `view.canInstall &&` condition because
`view.canInstall` is already guaranteed by the earlier early-return guard at lines
208-219. Refactor the section to be rendered only when `view.showCopyControls` is true,
keeping the rest of the section (title/help/command/code/copy button) unchanged. Ensure
the resulting rendered output is identical for all cases that reach this block.
1 participant
User description
Summary
installablelifecycle on catalog cards and detail routesScope
This is the frontend-only consumer PR from the multi-harness design. It is stacked on
davida-ps/catalog-installability-projection-v1(PR #311).Changed paths:
components/SkillCard.tsxpages/SkillDetail.tsxpages/SkillsCatalog.tsxtypes.tsutils/skillCatalogInstallability.mjsscripts/test-web-catalog-installability.mjsIt does not change Pages generation, any skill package, release policy, workflow, or publication path.
Security behavior
trueand legacy-absent lifecycle preserve the existing installation experiencefalseperforms no per-skill fetch and renders only labeled release/tag and checksum-manifest evidenceinstallable:falseadds a denial before checksum or documentation fetchesRemote validation
All executable validation ran only on
davida@20.14.133.241in preserved quarantine:/tmp/clawsec-web-catalog.Uzbumv/sourceThe remote checkout was pinned to projection head
ffe6281d809d980521a5375b7bf16777838cbe40. Only the six reviewed files were transferred, and all post-test SHA-256 values matched the local source.npm ci— 422 packages, 0 vulnerabilitiesnode scripts/test-web-catalog-installability.mjs— 64/64 assertions passedscripts/test-skill-*.mjs— passedfalse, legacy-absent Suitetruenpx eslint . --ext .ts,.tsx,.js,.jsx,.mjs --max-warnings 0— passednpx tsc --noEmit— passednpm run build— passed; 115 wiki exports generatedAn ephemeral Playwright/Chromium installation was created only under the remote quarantine, outside the repository. Final rendered checks passed:
Historical — not installableThe full skill-suite run used the previously reviewed narrow
zip -qr <archive> .compatibility wrapper with SHA-256472456191504d886c5871da3e900797512ab9266b8426d736c6a0f48248046b0because the remote host's systemzipis incompatible with the tag-release simulation.Release impact
No tags, releases, publications, store writes, or merges are performed by this PR.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Enforce the catalog
installablelifecycle acrossSkillCard,SkillsCatalog, andSkillDetailso install actions, platform affordances, and rendered documentation only appear for authorized skills. Centralize catalog/detail verification inskillCatalogInstallabilityto keep denied records as historical evidence and block malformed or missing direct routes.Modified files (3)
Latest Contributors(2)
Modified files (3)
Latest Contributors(2)