Repository navigation
fix(plugins): post-merge remediation for #33 (validator hardening, packaging hygiene, ask-codex metadata) - #34
Conversation
…rences Post-merge review of #33 (issue #10 hardening) found two P1 gaps: - readFrontmatterField/checkSkillFrontmatter tokenized frontmatter text instead of parsing YAML, so description: null/~/[]/{} or a bare comment all passed the required-description gate as truthy strings. Both validate-plugin-ports.mjs and validate-agent-skills.mjs now share scripts/lib/frontmatter.mjs, which parses the block with the yaml package and requires a trimmed non-empty string. - The SKILL.md reference guard skipped bare scripts/, cli/, assets/, hooks/ references entirely (issue #10 requires those shipped resources to exist) and joined paths without rejecting '..' or confirming containment, so a crafted tools/../../... reference could resolve to a file outside the plugin bundle. scripts/lib/bundle-refs.mjs now validates every unambiguous bundle reference, rejects traversal segments, and asserts the resolved path stays inside the plugin root. Widening bare-reference validation surfaced two genuine false positives (cross-skill delegation text in awesome-readme, a generic example in harness-audit) — reworded both so they no longer read as ambiguous same-bundle paths. Added table-driven regression fixtures for both (scripts/lib/frontmatter-test.mjs, scripts/lib/bundle-refs-test.mjs), wired into npm test.
Merge-SHA CI's npm run pack:dry included codex/plugins/html-docs/scripts/__pycache__/markdown_to_html.cpython-312.pyc (41.0 kB) — the html-docs fixture verifier imports markdown_to_html.py via importlib during its own junk-artifact scan step in validate-codex-plugins.mjs, and CPython writes the .pyc cache after that scan runs, so it survives into a later npm run pack:dry. - Pass PYTHONDONTWRITEBYTECODE=1 to the fixture-verifier subprocess so it is never generated in the first place. - Add a pack-content assertion in validate-package-contents.mjs (already wired into npm test) that fails the build if any __pycache__/ or *.py[cod] path ever appears in the npm pack --dry-run file list. - Add __pycache__/ and *.py[cod] to .gitignore for local git-status hygiene.
The rebase that squashed into #33 dropped 'Requires the codex binary on PATH' from plugin.json and marketplace.json while README.md and skills/ask-codex/SKILL.md kept it, breaking the four-surface consistency issue #10 requires. Restore the identical sentence to both, keeping the plugin.json/marketplace.json descriptions byte-for-byte equal (required by validate-plugin-ports.mjs's marketplace description check). Also replace marketplace.json's dead $schema (https://anthropic.com/claude-code/marketplace.schema.json, 404) with the live SchemaStore Claude Code marketplace schema. Editor-only, not a runtime blocker, but free to fix alongside the rest of this scope.
|
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThe pull request adds shared YAML frontmatter and bundle-reference utilities, strengthens skill and plugin validation, detects generated Python artifacts in packages, updates validation scripts, and revises plugin metadata and README image-generation instructions. ChangesValidation and plugin workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SkillValidator
participant FrontmatterParser
participant ReferenceUtilities
participant PluginFilesystem
SkillValidator->>FrontmatterParser: Parse SKILL.md frontmatter
FrontmatterParser-->>SkillValidator: Return name and description
SkillValidator->>ReferenceUtilities: Extract and resolve bundle references
ReferenceUtilities->>PluginFilesystem: Check file and symlink containment
PluginFilesystem-->>SkillValidator: Return validation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SHA-pinned remediation reviewReviewed exact range Findings
Passing evidence
|
…inment
Two P1 gaps from the SHA-pinned re-review:
- REF_EXT excluded ordinary asset types (png, svg, jpg, jpeg, gif, webp),
so a reference like assets/missing-logo.png never reached matching at
all — the prior bundle-refs-test.mjs fixture called
resolveBundleReference directly, never the actual regex extraction
used in production, so it never proved the real path worked.
extractBundleReferences moves into scripts/lib/bundle-refs.mjs
(shared, testable) and widens REF_EXT to cover real asset extensions
used in the repo's own plugins.
- A token-boundary bug let ${CLAUDE_PLUGIN_ROOT}/../tools/outside.ts get
rematched as the bare reference tools/outside.ts, silently dropping
the traversal prefix — a same-named file inside the bundle could then
make an escaping reference pass. extractBundleReferences now requires
a proper token boundary before a match (a negative lookbehind rejects
matches preceded by a word/path character) and captures leading ../
segments as part of the reference itself, so traversal is detected
downstream instead of disappearing.
Also closes the 'contained file' gap: resolveBundleReference is pure
again (no filesystem access, matching the lib/ boundary rule), only '..'
segments count as traversal (a safe scripts/./gen.sh no longer false-
positives), and the filesystem-facing checks (existence, regular-file,
symlink real-path containment) move to a new exported
checkReferenceOnDisk in validate-plugin-ports.mjs — the validator, not
the lib. A directory or an escaping symlink now fails even when the
lexical path looks contained.
validate-plugin-ports.mjs gains a guarded main() so its exported
checkReferenceOnDisk can be imported by tests without running a full
validation pass against the real repo.
Extended bundle-refs-test.mjs with extractor+resolver integration cases
(the exact assets/missing-logo.png and CLAUDE_PLUGIN_ROOT-traversal
scenarios from the review, including a same-named-file-inside-bundle
adversarial case) and checkReferenceOnDisk fixtures (directory, symlink
escape, safe symlink, safe dot-segment). Verified via stash: these tests
fail against the pre-fix code (a SyntaxError on the missing export).
The closing-delimiter search used indexOf("\n---"), which matches any
line merely *starting* with three dashes: ---not-a-delimiter, ----, and
--- trailing junk all silently closed the block early (or passed as a
'valid' close) instead of being rejected. Replace with a regex requiring
an exact standalone --- line (only trailing spaces/tabs before the
newline or end of string).
Added the three near-miss cases as negative fixtures. Verified via
stash: they fail against the pre-fix code with an actual assertion
failure ('Missing expected exception'), not just a load error.
…licitly
The prior round rewrote SKILL.md's bash scripts/gen.sh to bash gen.sh to
avoid a validator false positive, but that command never worked either:
no gen.sh exists at awesome-readme's own cwd (it belongs to the
delegated gpt-image-2 skill). The workflows still said bash
scripts/gen.sh, so SKILL.md and the workflows now disagreed on top of
both being wrong.
Remove the literal bash command blocks entirely in SKILL.md, Create.md,
and Improve.md and replace them with explicit parameter-list delegation
instructions to the gpt-image-2 skill, matching what the docs already
say to do ("invoke the gpt-image-2 skill directly"). This is a content
correctness fix, not a rewrite to dodge the reference matcher — with the
matcher already fixed to cover scripts/*.sh, a bare gen.sh path would
have been caught as either a false positive again or, if scoped to
scripts/, a real missing-file failure either way.
…k.json npm test (which ran validate-package-contents.mjs) executed before validate:plugins in the check script. The html-docs fixture verifier that generates .pyc lives inside validate:plugins, so a PYTHONDONTWRITEBYTECODE regression would pass the pack-content assertion first and only produce the artifact afterward — a later standalone npm run pack:dry would still exit zero with the bytecode included. Move the assertion to run after validate:plugins instead. Verified by manually reverting the PYTHONDONTWRITEBYTECODE fix: the reordered check now fails in the same run with 'Forbidden generated artifacts in package', where it previously would have passed silently. Also regenerate package-lock.json via npm install --package-lock-only so the yaml direct devDependency (added when the shared frontmatter parser landed) is reflected in the root packages[""].devDependencies block, matching package.json. CI's npm ci previously succeeded only because yaml@2.8.3 was already present transitively; npm ci now runs clean against the synced lockfile with a 2-line diff and no unrelated drift.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md (1)
133-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the
--refexample for consistency.
Create.mdandSKILL.mddocument the optional--refflag for reference-image conditioning. Since theImproveworkflow handles existing READMEs where a user might want to restyle an existing logo or diagram, it could be helpful to include the--refguidance here as well.💡 Proposed optional addition
- Comparison visual (if applicable): `--prompt "Side-by-side comparison: '<A>' vs '<B>'. Two columns, contrasting treatment, clear winner indicated." --out docs/comparison.png` +For reference-conditioned remixes, also pass `--ref /absolute/path/to/reference.png`. + Pass the user's prompt through raw. Only polish wording when they explicitly asked for it. Reference each output in the README with descriptive alt text. If image generation fails, name the failing layer in one sentence and ask the user how to proceed; do not silently fall back to a weaker model.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md` around lines 133 - 139, Update the image-generation guidance in the Improve workflow to document the optional --ref flag for reference-image conditioning, matching the existing guidance in Create.md and SKILL.md. Include it alongside the architecture, workflow, and comparison examples without changing the raw-prompt or failure-handling instructions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lib/bundle-refs.mjs`:
- Around line 13-17: Update the RegExp in extractBundleReferences to add a
negative lookahead immediately after the extension, preventing matches when
additional word or filename characters follow it (such as .bak or x). Add a
regression fixture covering these trailing-character cases and verify valid
references remain matched.
In `@scripts/validate-plugin-ports.mjs`:
- Around line 118-146: Update checkReferenceOnDisk to perform realpath
containment for every resolved target within the guarded filesystem operation,
not only when the final entry is a symlink. Handle realpathSync/statSync
failures from dangling or cyclic symlinks by returning the appropriate
validation reason instead of throwing, while preserving regular-file and
symlink-escape rejection. Add a regression test covering a target reached
through an escaping intermediate-directory symlink.
---
Nitpick comments:
In
`@claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md`:
- Around line 133-139: Update the image-generation guidance in the Improve
workflow to document the optional --ref flag for reference-image conditioning,
matching the existing guidance in Create.md and SKILL.md. Include it alongside
the architecture, workflow, and comparison examples without changing the
raw-prompt or failure-handling instructions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ab00732-2829-43c8-8ead-aa185d65f693
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (16)
.claude-plugin/marketplace.json.gitignoreclaude-code/plugins/ask-codex/.claude-plugin/plugin.jsonclaude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.mdclaude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Create.mdclaude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.mdclaude-code/plugins/harness-audit/skills/harness-audit/SKILL.mdpackage.jsonscripts/lib/bundle-refs-test.mjsscripts/lib/bundle-refs.mjsscripts/lib/frontmatter-test.mjsscripts/lib/frontmatter.mjsscripts/validate-agent-skills.mjsscripts/validate-codex-plugins.mjsscripts/validate-package-contents.mjsscripts/validate-plugin-ports.mjs
SHA-pinned fix re-reviewReviewed exact remediation delta Blocking findings
Verified closed / evidence
|
…uard
Two P1 gaps from the SHA-pinned re-review of the prior remediation:
- checkReferenceOnDisk only realpath-checked when the *final* path
component was itself a symlink (lstatSync().isSymbolicLink()). A
symlinked *parent* directory resolves away silently during normal path
resolution, so a plain regular file sitting behind one
(scripts/linked-dir -> <outside>, scripts/linked-dir/payload.md) was
seen as an ordinary contained file and returned {reason:"ok"}.
realpathSync now runs unconditionally on every candidate, and a
dangling or cyclic symlink (ENOENT/ELOOP) maps to a controlled
"missing" result instead of an uncaught throw.
- The direct-execution guard compared `import.meta.url ===
\`file://${process.argv[1]}\`` as raw strings. import.meta.url
percent-encodes characters like spaces; the interpolated argv string
does not, so a checkout path containing a space made the comparison
silently false and main() never ran — `node
validate-plugin-ports.mjs` would exit 0 having done nothing. Rebuilt
the comparison as native filesystem paths instead of URL strings
(fileURLToPath(import.meta.url) vs realpathSync(argv[1])), which also
fixes a second issue the first attempt at this introduced: macOS
resolves /tmp to /private/tmp, and import.meta.url reflects that
resolved path while path.resolve() alone does not follow the symlink.
Added symlinked-parent-directory-escape, dangling-symlink, and
cyclic-symlink fixtures to bundle-refs-test.mjs, and a new
cli-entrypoint-test.mjs that copies the real script into a temp
directory whose path contains a space and runs it as a subprocess,
asserting main() actually executed (non-zero exit, 'plugins directory
not found' on stderr) rather than silently exiting 0. Verified via
stash: the symlinked-parent case returns {reason:"ok"} and the
space-path smoke test exits 0 with no output against the pre-fix code.
Two more production-shape gaps in extractBundleReferences from the SHA-pinned re-review (also flagged independently by CodeRabbit): - The regex had a leading token boundary but no trailing one, so a coincidental prefix of a longer filename truncated into a match: assets/logo.png.bak extracted assets/logo.png, because the greedy path-character class backtracks to find any valid extension match rather than requiring the match to reach the actual end of the token. A same-prefixed real file could then validate text that was never actually a supported asset reference. Added a trailing negative lookahead so nothing word/dot/hyphen-like may immediately follow the matched extension. - ./scripts/gen.sh extracted nothing: the leading-boundary lookbehind correctly blocks resynchronization after a slash, but that also meant a safe, common ./ same-directory prefix was never recognized as part of a reference at all, a false negative on legitimate content. Widened the existing leading-segment group (already used to preserve traversal prefixes) so it captures ./ the same way it already captures ../. Extended bundle-refs-test.mjs (already touched in the prior commit) with the exact assets/logo.png.bak and ./scripts/gen.sh cases from the review, plus an end-to-end fixture proving a real assets/logo.png file in the bundle still can't be coincidentally matched by the .bak text. Verified via stash: assets/logo.png.bak produces 1 match (not 0) against the pre-fix regex.
Create.md and SKILL.md both document the optional --ref flag for reference-image conditioning; Improve.md, which handles existing READMEs where a user might want to restyle an existing logo or diagram, did not. Add the same one-line guidance for consistency.
SHA-pinned final re-reviewReviewed the exact three-commit delta Remaining blockerP1 — the terminal regex boundary still permits slash-tail truncation. If Verified closed / evidence
|
The terminal negative lookahead added for the .bak truncation bug was (?![\w.-]), omitting / even though / is itself part of the accepted path-character class. That left a second truncation path open: assets/logo.png/extra still extracted assets/logo.png, because a following slash satisfied the lookahead. If assets/logo.png exists, the validator approves that truncated prefix and never sees the actual longer token. Add / to the lookahead: (?![\w./-]). This is exactly what CodeRabbit's now-resolved thread proposed; the prior commit implemented the word/dot/hyphen half but dropped the slash. Added unit fixtures for assets/logo.pngx (no separator at all) and assets/logo.png/child.bin (the slash-tail case), plus an end-to-end production-path fixture proving the pipeline never extracts a match for the nested-path text even with a real assets/logo.png file present in the bundle. Verified via stash: the new slash-tail fixture extracts 1 match (not 0) against the pre-fix regex.
SHA-pinned final boundary re-reviewReviewed exact delta Boundary verification
Full gate evidence
|
1 participant
Post-merge remediation for #33
Follow-up to #33 (issue #10 plugin-port hardening). A heightened post-merge review of the merged squash range found four bounded blockers in the shipped validators/metadata. This PR closes all of them; no other content changed.
Review: #33 (comment)
1. Validator YAML/description false negatives (P1)
readFrontmatterField/checkSkillFrontmatterinvalidate-plugin-ports.mjstokenized frontmatter text instead of parsing YAML, sodescription: null,~,[],{}, and a bare comment all passed the required-description gate as truthy strings.validate-agent-skills.mjshad the same class of bug.Both validators now share
scripts/lib/frontmatter.mjs, which parses the frontmatter block with theyamlpackage and requires a trimmed non-empty string viaisNonEmptyString. Table-driven fixtures inscripts/lib/frontmatter-test.mjscover quoted/plain/folded valid values and null/collection/comment-only/malformed/empty invalid values, wired intonpm test.2. Bundle-reference coverage gap and path traversal (P1)
The
SKILL.mdreference guard deliberately skipped barescripts/,cli/,assets/,hooks/references even though issue #10's acceptance criteria explicitly require those shipped resources to exist, and it joined paths without rejecting..or confirming containment, so a craftedtools/../../...reference could resolve to a file outside the plugin bundle.scripts/lib/bundle-refs.mjsnow validates every unambiguous bundle reference, rejects traversal segments, and asserts the resolved path stays inside the plugin root. Widening bare-reference validation surfaced two genuine false positives:awesome-readme'sscripts/gen.shis a cross-skill delegation to the externalgpt-image-2skill, andharness-audit's./scripts/test.shis a generic illustrative example in a readiness rubric. Both are real, not bugs, so I reworded the two lines minimally so they no longer read as ambiguous same-bundle paths rather than suppressing the check. Fixtures for missing script/asset and path-escape cases are inscripts/lib/bundle-refs-test.mjs, wired intonpm test.3. Generated Python bytecode reaching the package (P1)
Merge-SHA CI's
npm run pack:dryincludedcodex/plugins/html-docs/scripts/__pycache__/markdown_to_html.cpython-312.pyc(41.0 kB): the html-docs fixture verifier importsmarkdown_to_html.pyviaimportlibduring its own junk-artifact scan step, and CPython writes the.pyccache after that scan runs, so it survives into a laternpm run pack:dry.validate-codex-plugins.mjsnow passesPYTHONDONTWRITEBYTECODE=1to the fixture-verifier subprocess so the cache is never generated.validate-package-contents.mjs(already wired intonpm test) now asserts no__pycache__/or*.py[cod]path ever appears in thenpm pack --dry-runfile list..gitignoregained__pycache__/and*.py[cod]for local git-status hygiene.4. ask-codex metadata drift (P2)
The rebase that squashed into #33 dropped "Requires the
codexbinary on PATH" fromplugin.jsonandmarketplace.jsonwhileREADME.mdandskills/ask-codex/SKILL.mdkept it, breaking the four-surface consistency issue #10 requires. Restored the identical sentence to both manifest and marketplace entry (kept byte-for-byte equal, pervalidate-plugin-ports.mjs's exact-match check). Also replaced marketplace.json's dead$schema(https://anthropic.com/claude-code/marketplace.schema.json, confirmed 404) with the live SchemaStore Claude Code marketplace schema (https://json.schemastore.org/claude-code-marketplace.json, confirmed valid draft-07 schema). Editor-only, not a runtime blocker.Evidence
All gates run with bun/bunx at head
bbd7dce90276af0309843324f4d4fcb4abcd722a:bun install: cleanbun run check(lint, typecheck, tests including the two new fixture suites,validate:plugins19/19,validate:skills34 SKILL.md files, list-resources): exit 0bun run pack:dry: exit 0, 232 files, no__pycache__/.pycin the listinggitleaks detect --no-git --config .gitleaks.tomlagainst the exact diff range (origin/main..HEAD, excluding the untracked localbun.lock): no leaksgitleaks protect --staged: no leaks (ran on each of the 3 commits via the repo's own pre-commit hook)pi -ne -e .: exit 0, all 10 repository extensions load cleanly (-neavoids this operator's machine-local global-extension-collision quirk documented separately in build(deps): bump ws from 8.20.0 to 8.21.1 in the npm_and_yarn group across 1 directory #31; unrelated to this PR's content)Verified the two widened false-positive fixtures fail without the content rewording and pass with it; verified the pack-content assertion actually catches a manually-planted
__pycache__file before removing it.Not merged, per assignment scope.
Refs #33, #10.
Summary by CodeRabbit
New Features
gpt-image-2skill, including support for prompts and reference images.codexcommand must be available onPATH.Bug Fixes
Tests