refactor(firecrawl): extract maintainer update/drift pipeline into a sibling skill

[kyle-sexton]

Summary

The firecrawl plugin bundled its maintainer-facing update/drift-gate pipeline (UPSTREAM.md, scripts/update.sh, context/update-flow.md, and the "Updating the skill and CLI" / "Safety" / Preservation-rules sections) inside the user-facing /firecrawl:firecrawl wrapper skill. Issue #261 asks to extract it into its own sibling skill.

Fix

Extracted the pipeline into a new sibling skill, /firecrawl:update (user-invocable, disable-model-invocation: true — maintainer-only), modeled on the existing plugins/playbooks/skills/update/SKILL.md standalone pattern:

• git mv'd UPSTREAM.md, scripts/update.sh (+ update.test.sh), and context/update-flow.md into plugins/firecrawl/skills/update/ (history preserved).
• Moved the maintainer prose (Preservation rules, Safety, update model) into the new skills/update/SKILL.md; the wrapper now carries only its user-facing scrape/search/crawl surface plus a pointer to the update skill.
• Invocation moves from /firecrawl:firecrawl update to /firecrawl:update; updated all references (wrapper skill, setup skill, the moved update.sh / update-flow.md, README). No behavior change to the update flow itself.
• Registered the new update skill-leaf-name collision with playbooks:update in scripts/skill-leaf-name-registry.txt (fixed verb per PLUGIN-PHILOSOPHY.md naming table).
• Bumped plugin.json 0.3.0 → 0.4.0; top-inserted a CHANGELOG entry.

Note: firecrawl already mirrored context7's lookup-vs-setup split (update was an action inside the main skill, as it still is in context7:lookup). This change goes one step further per the issue's explicit "extract into a sibling" ask, adopting the playbooks:update standalone pattern instead. Whether context7 should follow suit for cross-plugin consistency is left as a deferred question.

Verification

All run against changed files in the worktree:

• plugins/firecrawl/skills/update/scripts/update.test.sh — 17/17 checks passed
• scripts/check-skill-leaf-names.sh --check — all 9 collisions registered (incl. new update)
• scripts/check-changed-skills.sh origin/main — 3 skills checked, 0 failed (firecrawl/setup/update all PASS)
• scripts/check-skill-portability.sh origin/main — no unexcused coupling tokens
• shellcheck -x on update.sh + update.test.sh — clean
• markdownlint-cli2 on changed .md files — 0 errors

Related

• Follows the plugins/playbooks/skills/update/SKILL.md standalone maintainer-update pattern.
• Sibling plugins/firecrawl/skills/setup/SKILL.md already mirrors context7:setup.
• Deferred: consider extracting context7:lookup's update action into a context7:update sibling for cross-plugin consistency.

Closes #261

[claude]

Claude finished @kyle-sexton's task in 3m 19s —— View job

---

Security Review — PR #743

• Gather PR diff and changed files
• Review shell scripts (update.sh, update.test.sh) for injection/path traversal
• Review SKILL.md files for unsafe invocation patterns
• Review plugin.json for supply-chain / config risks
• Review update-flow.md and UPSTREAM.md for unsafe patterns
• Post findings

---

Findings

IMPORTANT — user-invocable: true allows any consumer to trigger a global npm install (confidence: PLAUSIBLE)

Files:

• plugins/firecrawl/skills/update/SKILL.md line 6
• plugins/firecrawl/skills/update/scripts/update.sh line 188

The skill is labelled "maintainer-only" and "Do NOT auto-fire this skill" throughout the prose, but user-invocable: true (line 6) makes it appear in the skill picker and fully invocable by any consumer who has installed the plugin. disable-model-invocation: true only prevents the model from autonomously firing it — a user who types /firecrawl:update (no --check) and approves Gate 1 will have npm install -g firecrawl-cli@latest run globally on their system (update.sh line 188). The two approval gates are conversational, not access-control-enforced.

The blast radius is bounded (user must explicitly invoke the skill and explicitly approve), but:

1. A confused consumer could upgrade their global firecrawl-cli unexpectedly.
2. If the npm package were compromised at the moment of invocation, --apply would install it without any content verification beyond the post-install firecrawl --status check.

Recommendation: Either flip user-invocable to false and document an alternative invocation path for actual maintainers (e.g., direct bash scripts/update.sh --apply from a checkout), or add a preamble guard in the skill body that prints a prominent warning and exits early unless the user is operating from a recognized working-tree checkout (e.g., checking for a git root or the marketplace clone marker). The current SKILL.md-existence check in main() does not help — SKILL.md IS present in an installed marketplace copy.

---

SUGGESTION — Unquoted heredoc in rewrite_upstream_md enables variable/command expansion (confidence: CONFIRMED)

File: plugins/firecrawl/skills/update/scripts/update.sh line 226

cat >"$UPSTREAM_MD" <<EOF

The unquoted <<EOF delimiter means $(), backtick, and ${…} expressions inside the heredoc body are shell-expanded at write time. Current variable sources are safely typed — $sha is a hex digest, $ver/$prev are filtered through grep -oE '[0-9]+\.[0-9]+\.[0-9]+', $today is date -u +%Y-%m-%d, and $UPSTREAM_URL/$NPM_PKG are hardcoded constants — so there is no current injection path. Replacing <<EOF with <<'EOF' would eliminate the surface by design without changing any observable behaviour, since only literal text and these typed variables appear in the body.

---

No findings in

• update.test.sh — $SCRIPT is derived from ${BASH_SOURCE[0]} (not user input); sourcing it is safe.
• plugin.json — version bump only; no new dependencies or loosened pins.
• Pre-computed context commands — ${CLAUDE_SKILL_DIR} is a runtime-supplied system variable; the grep is read-only against a file in the skill's own directory.
• UPSTREAM.md fetch — content is hashed only and never executed; a corrupted upstream file would be reviewed behind Gate 2 before any content is integrated.
• allowed-tools glob — Bash(grep -m1 *UPSTREAM.md*) appropriately scopes the single pre-computed context command.

[claude]

Claude finished @kyle-sexton's task in 2m 45s —— View job

---

PR Review — refactor(firecrawl): extract maintainer update/drift pipeline into a sibling skill

• Gather context and understand the request
• Review changed files
• Check plugin.json version bump and CHANGELOG
• Check skill structure and SKILL.md content
• Verify cross-references between skills
• Check registry and portability concerns
• Post findings

---

Summary

Clean refactor. The structural split (moving UPSTREAM.md, update.sh, update.test.sh, and context/update-flow.md into skills/update/ with full git history) is well-executed. The new skill correctly mirrors the playbooks:update standalone pattern: user-invocable: true + disable-model-invocation: true, a CLAUDE_SKILL_DIR-relative pre-computed context, and allowed-tools scoped to just the pre-computed grep. Registry entry, version bump, and cross-references are all correct. Two minor issues below.

---

Findings

1. Stale section name in two log messages — update.sh lines 174 and 218

update.sh was moved from skills/firecrawl/scripts/ into skills/update/scripts/. It still references the old section title 'Updating the skill and CLI', which no longer exists in the update skill's own SKILL.md. The correct section in skills/update/SKILL.md is 'Preservation rules'. A maintainer reading these log messages during a drift run will look in the wrong place.

Line 174 (permalink):

log "governed by the Preservation rules in SKILL.md 'Updating the skill and CLI'."

Should be:

log "governed by the Preservation rules in this skill's SKILL.md."

Line 218 (permalink):

log "Preservation rules. See SKILL.md 'Updating the skill and CLI'."

Should be:

log "Preservation rules. See this skill's SKILL.md 'Preservation rules'."

Fix this →

2. CHANGELOG entry missing date

The [0.4.0] entry (permalink) has no date, while all prior entries ([0.3.0], [0.2.2], [0.2.1]) include a — YYYY-MM-DD suffix per Keep-a-Changelog convention. Minor, but breaks the pattern.

## [0.4.0] — 2026-07-20

Fix this →

---

Everything else looks good

• File moves: git mv paths are correct; UPSTREAM.md, update.sh, update.test.sh, and context/update-flow.md all land in skills/update/ as expected.
• Skill frontmatter: disable-model-invocation: true + user-invocable: true is the right combination to express "manually invocable, never auto-fired," matching playbooks:update.
• allowed-tools: Correctly trimmed to only Bash(grep -m1 *UPSTREAM.md*) for the pre-computed !grep context line; the removed Bash(firecrawl --status*) was for the wrapper and is gone.
• CLAUDE_SKILL_DIR resolution: The pre-computed context in update/SKILL.md uses ${CLAUDE_SKILL_DIR}/UPSTREAM.md, which resolves to skills/update/UPSTREAM.md — correct.
• update-flow.md: Both invocation table entries updated from /firecrawl:firecrawl update to /firecrawl:update. Header reference updated to "Preservation rules" + "Safety".
• Registry entry (skill-leaf-name-registry.txt): Comment, owner pair (firecrawl,playbooks), and placement are correct.
• setup/SKILL.md: The "What this skill does NOT do" pointer updated from /firecrawl:firecrawl update to /firecrawl:update.
• plugin.json description: Updated to reflect the three-skill surface (wrapper, setup, update). Version bump 0.3.0 → 0.4.0 is appropriate for a new skill leaf.

[claude]

Claude finished @kyle-sexton's task in 3m 34s —— View job

---

PR Review

• Gather context and understand the request
• Review changed files
• Check plugin.json version bump and CHANGELOG
• Check skill structure and SKILL.md content
• Verify cross-references between skills
• Check registry and portability concerns
• Post findings

---

Summary

This is a clean, well-scoped refactor with a clear separation of concerns. No blocking issues found. Two minor observations below.

---

What's correct

Structure and file layout — plugins/firecrawl/skills/update/ follows the playbooks:update standalone pattern precisely: UPSTREAM.md, scripts/update.sh, scripts/update.test.sh, and context/update-flow.md all landed in the right place with history preserved.

firecrawl/SKILL.md cleanup — The wrapper now carries only user-facing surface area plus a pointer to /firecrawl:update (lines 119–122, 136). The Preservation rules, Safety section, and update model are gone from the wrapper, which is the goal of the issue.

update.sh shell safety — No injection paths. NPM_PKG is a hardcoded literal (line 29). rewrite_upstream_md interpolates only a SHA256 hash, a semver string (grep-filtered), and a previous semver — all sanitized by the code that produces them. --fail on all curl calls (lines 91, 102) prevents silent error-page poisoning of UPSTREAM.md. TMPDIR_RUN cleanup via trap EXIT (line 39) is reliable.

Test coverage — update.test.sh covers all helper functions (recorded_field, rewrite_upstream_md, sha256), the CLI arg contract (--help, -h, unknown flag), and the source-guard, all network-free. 17/17 reported passing.

Cross-references — All updated consistently: wrapper skill (lines 119–122, 136–137), setup/SKILL.md line 95 (/firecrawl:update pointer), plugin README.md, root README.md, update-flow.md invocation table, update.sh drift-detected message (line 172).

Registry entry — scripts/skill-leaf-name-registry.txt lines 64–68 correctly registers update firecrawl,playbooks with its rationale. The owner set matches the actual collision.

Version and CHANGELOG — 0.3.0 → 0.4.0 is appropriate (new skill, invocation path change). CHANGELOG entry is correctly top-inserted and scoped.

---

Minor observations (non-blocking)

1. allowed-tools glob covers the grep call but the pre-computed command also pipes through sed

skills/update/SKILL.md line 7:

allowed-tools: Bash(grep -m1 *UPSTREAM.md*)

The pre-computed context block (line 12) runs:

grep -m1 '^- Last sync:' "${CLAUDE_SKILL_DIR}/UPSTREAM.md" 2>/dev/null | sed 's/^- //' || echo "never…"

Whether allowed-tools matches against the full pipeline string or only the initial command depends on the platform implementation. If it matches the full string, sed would be unmatched. The fallback (|| echo "never…") handles a failure gracefully, so in the worst case the pre-computed line reads "never — run this skill with --check" instead of the real date. Worth verifying against the setup skill's pre-computed pattern to confirm the convention.

2. check_prereqs calls exit 2 rather than return

scripts/update.sh lines 54–67. When sourced, exit terminates the sourcing shell rather than the function. The test file correctly avoids ever calling check_prereqs or main when sourcing, so this is safe as written. It's worth a comment if the test is ever extended — the existing source-guard comment (line 291) covers the main side but not the exit-in-helper subtlety.

---

Verdict: Approve. The structural refactor is sound, shell scripts are clean, test coverage is solid, and all cross-references are consistent. The two observations above are low-severity and don't block merge.

[claude]

Claude finished @kyle-sexton's task in 4m 10s —— View job

---

Security Review — PR #743

• Gather PR diff and changed files
• Review shell scripts (update.sh, update.test.sh) for injection/path traversal
• Review SKILL.md files for unsafe invocation patterns
• Review plugin.json for supply-chain / config risks
• Review update-flow.md and UPSTREAM.md for unsafe patterns
• Post findings

---

Findings

IMPORTANT — user-invocable: true makes a global npm install picker-visible to every consumer (confidence: CONFIRMED)

Files:

• plugins/firecrawl/skills/update/SKILL.md line 5
• plugins/firecrawl/skills/update/scripts/update.sh line 188

The new skill is explicitly labelled "maintainer-only" in its description and body, but user-invocable: true registers it in the skill picker for every consumer who has the plugin installed. Any consumer can type /firecrawl:update and — if they approve Gate 1 — have npm install -g "${NPM_PKG}@latest" run globally on their system.

Net change vs. the pre-PR state: This PR is overall an improvement — before this refactor, the update action was a subcommand of the main /firecrawl:firecrawl skill which had disable-model-invocation: false, meaning the model could autonomously fire the update if it inferred that was appropriate. The new disable-model-invocation: true eliminates that autonomous-fire risk. However, the skill is now explicitly picker-visible with a clear label and description, whereas previously it was buried inside the wrapper skill's argument surface.

The blast radius remains bounded (user must explicitly type the skill and approve Gate 1), but:

1. A confused consumer could upgrade their global firecrawl-cli unexpectedly.
2. At the moment of a compromised npm package, --apply installs it with only the post-install firecrawl --status check as verification — no content hash or signature check.

Recommendation: Flip user-invocable: false. Actual maintainers who need to invoke this from a checkout can run bash scripts/update.sh [--check|--apply] directly, or invoke the skill via --plugin-dir from a working tree. Consumers have no legitimate reason to see or invoke this skill, and the gate-based guardrails are not a substitute for access control.

If user-invocable: true must stay (e.g., because the plugin framework requires it for invocability), add a preamble guard in the skill body that confirms the invoker is operating from a working-tree checkout — e.g., by checking for a .git directory at ${CLAUDE_SKILL_DIR}/../../../../.git — and exits early with a clear message if not.

Fix this →

---

SUGGESTION — Unquoted heredoc enables silent variable/command expansion (confidence: CONFIRMED)

File: plugins/firecrawl/skills/update/scripts/update.sh line 226

cat >"$UPSTREAM_MD" <<EOF

The unquoted delimiter <<EOF expands ${}, $(), and backticks inside the heredoc body at write time. All current variables in the body are safe — $sha is hex-only (from awk '{print $1}' applied to SHA256 output), $ver/$prev are filtered through grep -oE '[0-9]+\.[0-9]+\.[0-9]+', $today is date -u +%Y-%m-%d, and $UPSTREAM_URL/$NPM_PKG are script-level constants. There is no current injection path.

Quoting the delimiter (<<'EOF') would eliminate the expansion surface entirely without changing any observable behaviour, and makes it safer to extend the heredoc body in the future.

Fix this →

---

No findings in

• update.test.sh — $SCRIPT is derived from ${BASH_SOURCE[0]}; sourcing it is safe.
• plugin.json — version bump only; no new dependencies or loosened pins.
• UPSTREAM.md — content is hashed and never executed; written only by typed-variable heredoc in update.sh.
• update-flow.md — procedural doc only; no executable surfaces.
• Pre-computed context command — ${CLAUDE_SKILL_DIR} is a system-supplied runtime variable; the grep is read-only against a file in the skill's own directory; the sed strips only a fixed prefix.
• curl fetches — --fail flag on both curl calls (NPM_LATEST_URL and UPSTREAM_URL) correctly prevents silent HTTP-error-page-as-content poisoning.
• npm registry response parsing — jq -r '.version // empty' output is used only for display and string comparison; it is not passed to npm install (which uses the hardcoded @latest tag), so a malicious registry response cannot influence what gets installed.
• SKILL.md existence guard (line 247) — ineffective at distinguishing installed vs. working-tree copies since SKILL.md is present in both, but this is pre-existing behaviour, not introduced by this PR.

[kyle-sexton]

Post-green review classification (rule 8)

Finding	Classification	Resolution
user-invocable: true on picker-visible maintainer-only skill (initial security-review pass)	REVERSED — not a defect	Matches established repo convention: user-invocable: true + disable-model-invocation: true = picker-visible, auto-invoke-blocked. Confirmed identical pattern in plugins/context7/skills/setup/SKILL.md and plugins/playbooks/skills/update/SKILL.md. Setting user-invocable: false here would combine with the skill's own disable-model-invocation: true to make it permanently unreachable by anyone, per official skill-frontmatter semantics.
Unquoted <<EOF heredoc in rewrite_upstream_md	REVERSED — not a defect	Heredoc must stay unquoted: it interpolates ${sha}/${ver}/${prev}/${today} into UPSTREAM.md for drift tracking. Quoting (<<'EOF') would write literal ${...} strings and fail update.test.sh's section-5 assertions (lines 100-104), which assert the expanded values appear in the written file.

Re-run of security-review at current HEAD is clean (no findings). Independently verified both reversals against live repo state and the test file before accepting. Flipping to ready and handing off to babysit — no further re-polling from this lane.

[claude]

Claude finished @kyle-sexton's task in 3m 22s —— View job

---

Security Review — PR #743

• Gather PR diff and changed files
• Review update.sh for injection/path traversal/command execution risks
• Review SKILL.md files for unsafe invocation patterns
• Review plugin.json and registry changes for supply-chain risk
• Review update-flow.md and UPSTREAM.md for unsafe patterns
• Post findings

---

Net security posture: improvement over the previous state

Before this PR, the update action lived inside /firecrawl:firecrawl, which carried disable-model-invocation: false. The model could autonomously fire the update (including the npm install -g path) if it judged that appropriate. After this PR, the update skill carries disable-model-invocation: true, eliminating that autonomous-fire risk. The picker visibility trade-off is real but bounded — see below.

---

Findings

SUGGESTION — user-invocable: true on a skill whose --apply path runs npm install -g (confidence: PLAUSIBLE, bounded blast radius)

Files:

• plugins/firecrawl/skills/update/SKILL.md line 5
• plugins/firecrawl/skills/update/scripts/update.sh line 188

Any consumer who has the plugin installed can type /firecrawl:update and — upon approving Gate 1 — execute npm install -g "${NPM_PKG}@latest" globally on their system.

Why this is bounded:

• The user must explicitly invoke the skill and then explicitly approve Gate 1. No ambient trigger.
• NPM_PKG is a hardcoded constant (firecrawl-cli); no injection path exists from user input or a malicious npm registry response to influence which package is installed — the install command is npm install -g "${NPM_PKG}@latest", not npm install -g "${latest_version}" (the version-string from the registry is used only for comparison/display).
• The skill description clearly labels this as "Not for consumers"; the frontmatter description is the first thing a picker-aware consumer reads.
• This pattern (user-invocable: true + disable-model-invocation: true) is established repo convention, confirmed in plugins/context7/skills/setup/SKILL.md and plugins/playbooks/skills/update/SKILL.md. Setting user-invocable: false with disable-model-invocation: true would make the skill permanently unreachable.

Residual risk: A compromised firecrawl-cli package on the npm registry at the moment a consumer invokes --apply would be installed. The post-install firecrawl --status check (line 200) is a sanity check, not a content-verification or signature check. This is inherent to any @latest npm install and is not specific to this PR.

No action required. Noting for completeness; the previous reversal stands.

---

No findings in

• Injection paths in update.sh — All variable sources are typed. NPM_PKG and UPSTREAM_URL are hardcoded constants. $sha is the first field of SHA-256 output (hex only). $ver/$prev are filtered through grep -oE '[0-9]+\.[0-9]+\.[0-9]+'. $today is date -u +%Y-%m-%d. jq -r '.version // empty' output goes only to string comparisons and log output — it is never passed to a shell-expanding context. The unquoted <<EOF in rewrite_upstream_md (line 226) expands only these typed variables; quoting to <<'EOF' would break the file format and the tests (section-5 assertions check for expanded values), so the unquoted form is correct.
• Path traversal — SCRIPT_DIR is derived from ${BASH_SOURCE[0]} (bash-controlled), not user input. SKILL_DIR is ${SCRIPT_DIR}/.. (canonical via cd … && pwd). UPSTREAM_MD is ${SKILL_DIR}/UPSTREAM.md — no traversal possible from user input.
• curl poisoning — Both curl calls (NPM_LATEST_URL, UPSTREAM_URL) use --fail (lines 91, 102) and --max-time. HTTP error-page bodies cannot be silently saved as UPSTREAM.md.
• Tmp cleanup — cleanup is registered via trap cleanup EXIT (line 39); TMPDIR_RUN is reliably removed on all exit paths. The fetched upstream content is never executed — hashed only.
• plugin.json — Version bump (0.3.0 → 0.4.0) only; no new dependencies, no loosened pins, no new external registries.
• skill-leaf-name-registry.txt — Adds update firecrawl,playbooks with a comment explaining the accepted collision. No executable surface.
• update.test.sh — $SCRIPT derived from ${BASH_SOURCE[0]}; no network calls; sourcing update.sh is safe because main "$@" is source-guarded (line 291).
• GitHub Actions — No workflow files changed.
• SKILL.md pre-computed context — Read-only grep against a file inside the skill's own directory; ${CLAUDE_SKILL_DIR} is a system-supplied variable. No write surface.

[claude]

Claude finished @kyle-sexton's task in 2m 34s —— View job

---

PR Review — refactor(firecrawl): extract maintainer update/drift pipeline into a sibling skill

• Gather PR diff and changed files
• Review new skills/update/SKILL.md structure and frontmatter
• Review update.sh for correctness (stale references flagged in prior reviews)
• Review CHANGELOG.md for date-format consistency
• Review cross-references across all changed files
• Check plugin.json version bump and registry entry
• Post findings

---

Summary

Clean, well-scoped refactor. The structural split is sound, all cross-references are consistent, and shell scripts are safe. One prior finding was already fixed by a follow-up commit; one minor issue remains open.

---

Status of prior review findings

Stale section-name references in update.sh (flagged by the first review pass) — RESOLVED by commit 893b627. Lines 174 and 218 now correctly read:

"governed by this update skill's SKILL.md Preservation rules."
"Preservation rules. See this update skill's SKILL.md."

Security findings (user-invocable: true + unquoted <<EOF) — both correctly reversed by @kyle-sexton with solid rationale: user-invocable: true is required to make a skill invocable at all (matches playbooks:update and context7:setup), and the heredoc must stay unquoted for ${sha} / ${ver} / ${prev} interpolation (quoting it would break update.test.sh section-5 assertions).

---

Remaining issue — CHANGELOG date still missing

File: plugins/firecrawl/CHANGELOG.md line 6

The [0.4.0] header has no date, while every prior entry ([0.3.0], [0.2.2], [0.2.1]) includes a — YYYY-MM-DD suffix per Keep-a-Changelog convention:

-## [0.4.0]
+## [0.4.0] — 2026-07-20

Fix this →

---

Everything looks good

• File moves — UPSTREAM.md, update.sh, update.test.sh, context/update-flow.md all land in skills/update/ per the PR description; history preserved.
• update/SKILL.md frontmatter — user-invocable: true + disable-model-invocation: true matches the playbooks:update pattern exactly; allowed-tools correctly scoped to Bash(grep -m1 *UPSTREAM.md*) for the pre-computed context line.
• Preservation rules — the disable-model-invocation: false entry on line 45 correctly documents the wrapper skill's required frontmatter, not the update skill's own — no mismatch.
• update.sh shell safety — no injection paths: NPM_PKG is a hardcoded literal; ${sha} is hex-only from awk '{print $1}' on SHA256 output; ${ver}/${prev} filtered through grep -oE '[0-9]+\.[0-9]+\.[0-9]+'; ${today} from date -u +%Y-%m-%d; --fail on both curl calls correctly rejects HTTP error pages.
• update-flow.md — both invocation-table entries updated from /firecrawl:firecrawl update to /firecrawl:update; header reference points at "Preservation rules" + "Safety".
• setup/SKILL.md — pointer updated from /firecrawl:firecrawl update to /firecrawl:update.
• Wrapper SKILL.md — "Updating the skill and CLI" section now only contains the pointer to /firecrawl:update; maintainer prose removed.
• skill-leaf-name-registry.txt — new update firecrawl,playbooks entry at lines 64–68 with correct rationale; owner set sorted.
• plugin.json — 0.3.0 → 0.4.0 is appropriate for a new skill leaf; description reflects three-skill surface.

Verdict: Approve pending the one-line CHANGELOG date fix.

[kyle-sexton]

Babysit classification pass — canonical-token table (current HEAD 893b627)

Re-classifying every reviewer finding in the recognized VALID / INCORRECT / UNCERTAIN vocabulary (my earlier reversal used "REVERSED", which the mechanical readiness gate does not recognize). Verified against live repo state, not the bot assertions.

#	Finding	Classification	Evidence	Reacted
1	Security: user-invocable: true makes the maintainer-only skill picker-visible with an npm install -g path	INCORRECT	Established repo convention: user-invocable: true + disable-model-invocation: true = picker-visible, auto-invoke-blocked. Identical pattern in plugins/context7/skills/setup/SKILL.md and plugins/playbooks/skills/update/SKILL.md. Flipping to false alongside disable-model-invocation: true makes the skill permanently unreachable per skill-frontmatter semantics. Security-review re-run (comment 5026328481) itself downgraded to SUGGESTION / "no action required — the previous reversal stands."	👎
2	Security: unquoted <<EOF heredoc in rewrite_upstream_md enables shell expansion	INCORRECT	The heredoc must stay unquoted: it interpolates ${sha}/${ver}/${prev}/${today} into UPSTREAM.md. All sources are typed (hex digest, grep -oE '[0-9]+\.[0-9]+\.[0-9]+', date -u), so there is no injection path. Quoting (<<'EOF') would write literal ${...} and fail update.test.sh section-5 assertions, which assert the expanded values appear in the file.	👎
3	PR-review: stale section-name refs in update.sh lines 174/218 ('Updating the skill and CLI')	VALID — already fixed	Resolved in current HEAD 893b627; both log lines now point at this update skill's own SKILL.md Preservation rules. PR-review re-run (comment 5026328993) confirms "RESOLVED by commit 893b627."	👍
4	PR-review: CHANGELOG [0.4.0] header missing a — YYYY-MM-DD date	INCORRECT	The stated premise ("all prior entries include a date suffix per Keep-a-Changelog convention") is false. 0 of 358 version headers across every plugins/*/CHANGELOG.md in this repo carry a date suffix — firecrawl's own [0.3.0]/[0.2.2]/[0.2.1] are all dateless. Adding a date to [0.4.0] alone would make it the sole dated header in the repo, manufacturing the exact inconsistency the finding claims to prevent. The repo convention is deliberately dateless headers; no file-wide gap to defer.	👎
5	PR-review observation: allowed-tools glob may not cover the sed in the pre-computed pipeline	INCORRECT	Non-issue by design. The pre-computed line ends in `
6	PR-review observation: check_prereqs calls exit 2 rather than return when sourced	INCORRECT	Safe as written, as the reviewer itself noted. The source-guard (update.sh line 291) runs only main when sourced; update.test.sh never sources-then-invokes check_prereqs or main, so exit 2 cannot terminate the test harness.	👍

No code change required at this HEAD — all findings are INCORRECT (with evidence) or already fixed in 893b627. CI is fully green and mergeStateStatus is CLEAN.

[claude]

Claude finished @kyle-sexton's task in 2m 52s —— View job

---

Security Review — PR #743

• Get PR diff at current HEAD (2e6ea44)
• Read changed shell scripts (update.sh, update.test.sh)
• Read changed SKILL.md files
• Read plugin.json, CHANGELOG.md, and registry changes
• Check merge commit for any new content vs prior reviews
• Post findings

---

Net security posture: improvement over the previous state

The update action was previously buried inside /firecrawl:firecrawl with disable-model-invocation: false, meaning the model could autonomously invoke the npm install -g path. The new update skill carries disable-model-invocation: true, eliminating that autonomous-fire risk. The merge commit (2e6ea44) brought only main-branch files (runner policy, workflow YAML, lychee config) — none of these appear in the PR diff against origin/main and none are firecrawl-related.

---

No security findings

Injection surfaces in update.sh — all clean.

• NPM_PKG (line 29) is a hardcoded constant. No user input, environment variable, or network response can change what package gets installed — npm install -g "${NPM_PKG}@latest" always resolves to firecrawl-cli@latest via npm's own resolution, not from the registry-returned version string.
• $sha (line 106) is filtered through awk '{print $1}' on SHA256 output — hex characters only.
• $ver / $prev (line 82) are filtered through grep -oE '[0-9]+\.[0-9]+\.[0-9]+' — semver digits only.
• jq -r '.version // empty' output (line 93) is used only for display and string comparison — never passed to a shell-expanding context.
• recorded_field (line 111) is called only with hardcoded string literals; the grep pattern is not user-controlled.

curl poisoning — both calls (NPM_LATEST_URL at line 91, UPSTREAM_URL at line 102) use --fail, preventing HTTP error-page bodies from being silently saved or hashed as real content. The upstream SKILL.md is fetched to a temp file, hashed only, and never executed.

Path traversal — SCRIPT_DIR derives from ${BASH_SOURCE[0]} (bash-controlled, not user input); SKILL_DIR is ${SCRIPT_DIR}/.. resolved via cd && pwd; UPSTREAM_MD is ${SKILL_DIR}/UPSTREAM.md. No traversal path from any user-controlled input.

Temp cleanup — TMPDIR_RUN is created with mktemp -d and removed by trap cleanup EXIT (line 39), covering all exit paths including error exits.

user-invocable: true / unquoted <<EOF — both prior findings correctly reversed by @kyle-sexton with solid evidence: user-invocable: true is required for a skill to be invocable at all (identical pattern to playbooks:update and context7:setup); the heredoc must stay unquoted because it interpolates typed variables into UPSTREAM.md, and quoting would break the section-5 test assertions. The reversals hold.

update.test.sh — $SCRIPT is derived from ${BASH_SOURCE[0]}; sourcing update.sh is safe because main "$@" is source-guarded (line 291). No network calls in the test suite.

plugin.json — version bump (0.3.1 → 0.4.0) and description update only; no new dependencies, no loosened pins, no new external registries.

skill-leaf-name-registry.txt — adds a comment and update firecrawl,playbooks entry; no executable surface.

SKILL.md pre-computed context — read-only grep against a file inside the skill's own directory; ${CLAUDE_SKILL_DIR} is a system-supplied runtime variable.

Residual supply-chain risk (pre-existing, not introduced by this PR): npm install -g "${NPM_PKG}@latest" installs without a content hash or signature check. A compromised firecrawl-cli package on the npm registry at the moment of --apply invocation would be installed. This is inherent to any @latest npm install, requires explicit Gate 1 approval by the user, and existed in the previous state of the plugin.

---

Verdict: No security issues found. The refactor is clean and the net change is a security improvement.

[claude]

Claude finished @kyle-sexton's task in 2m 19s —— View job

---

PR Review — refactor(firecrawl): extract maintainer update/drift pipeline into a sibling skill

• Gather context and understand the request
• Inspect merge commit and diff vs origin/main
• Verify merge conflict resolutions
• Confirm CI status
• Post findings

---

Summary

This sync review covers the merge commit 2e6ea44, which incorporated origin/main (a2e28fd) into the PR branch. The merge noted two conflicts; both are correctly resolved. CI is green. All prior review findings are either fixed or correctly reversed with documented rationale. No new issues found.

---

Merge conflict resolutions — both correct

The merge commit records two conflict files:

plugins/firecrawl/.claude-plugin/plugin.json

• origin/main carried "version": "0.3.1" (a docs-only patch released after this branch was cut).
• The PR branch carried "version": "0.4.0" (this refactor — new skill leaf, minor bump).
• Resolution: 0.4.0 ✅ The PR's version is semantically higher and correct.

plugins/firecrawl/CHANGELOG.md

• origin/main added a [0.3.1] entry (LICENSE section change — docs only).
• The PR branch added a [0.4.0] entry (this refactor).
• Resolution: [0.4.0] → [0.3.1] → [0.3.0] ✅ Descending version order preserved. The 0.3.1 patch now correctly sits between the pre-branch base (0.3.0) and this PR's minor bump (0.4.0), which is accurate version history.

---

CI status — all green

Check	Conclusion
ci	✅ success
pr-title	✅ success
pr-issue-linkage	✅ success
do-not-merge	✅ success

---

Prior findings — all resolved or correctly reversed

#	Finding	Status
Stale section-name refs in update.sh lines 174/218	Fixed in 893b627 — log lines now reference "this update skill's SKILL.md Preservation rules"
Security: user-invocable: true on npm install -g path	Correctly reversed — established repo pattern (user-invocable: true + disable-model-invocation: true); false + true makes the skill permanently unreachable
Security: unquoted <<EOF heredoc	Correctly reversed — must stay unquoted to interpolate ${sha}/${ver}/${prev} into UPSTREAM.md; quoting would break test assertions
CHANGELOG [0.4.0] missing date	Correctly reversed — zero entries across all plugins/*/CHANGELOG.md use a date suffix; adding one here would create the inconsistency the finding claimed to prevent

---

Verdict: Approve. Merge conflict resolutions are correct, CI is clean, and all review findings from prior passes are either fixed or reversed with solid evidence. Ready to merge.