From a1169e3db779519d59a25bf7a8b1141a7256164a Mon Sep 17 00:00:00 2001 From: Louis Choquel Date: Thu, 17 Sep 2026 01:14:54 +0200 Subject: [PATCH 1/2] Keep argument placeholders out of skill bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude Code replaces $ARGUMENTS and $0, $1, … in a SKILL.md with the words the skill was invoked with, so pipelex-scaffold's dev-server block reached the model as a different command: its process-tree helper signalled "me" and the listener check printed "The", which would have stopped a loopback-bound server and reported no URL. The helper now loops over its arguments with a local variable, and the listener's address is read with lsof -Fn rather than by column, since lsof prints the socket state after the address. make check now fails when any target's rendered SKILL.md carries $ARGUMENTS or $. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013DQGXP7o1q5tm6LJLfSsRE --- CHANGELOG.md | 1 + docs/decisions.md | 7 ++ .../skills/pipelex-scaffold/SKILL.md | 4 +- pipelex-vibe/skills/pipelex-scaffold/SKILL.md | 4 +- pipelex/skills/pipelex-scaffold/SKILL.md | 4 +- scripts/check.py | 28 ++++++++ templates/skills/pipelex-scaffold/SKILL.md.j2 | 4 +- tests/unit/test_check.py | 68 +++++++++++++++++++ 8 files changed, 112 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a39cffaa..787f0688 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - **`pipelex-inputs` delegates file generation instead of doing it inline.** (Breaking) Its Document Generation section and Fallback Strategy block are gone, and the `native.Image` / `native.Document` strategy rows now delegate to `pipelex-synthetic-inputs`. "Generate File Inputs" is the delegation contract — the request fields to pass, the bare path that comes back and goes into `inputs.json`, and what happens when the factory cannot deliver: that one input is left unfilled with the reason in the report, and the rest of the flow continues. Anyone relying on the inline recipes should read them in the new skill's `references/`, where they are deeper and now executed by tests. - **Tooling:** Pinned `ruff` to an exact `0.16.4`, replacing the `>=0.6.8` floor. The exact pin matches what the Ruff VS Code extension now bundles, which matters because Ruff 0.16 lints `pyproject.toml` itself: the extension syncs the config file to the language server, and a pre-0.16 binary parses it as Python source and paints phantom `invalid-syntax` diagnostics on lines like `requires-python`. A floor let the editor and the CLI resolve to different binaries; an exact pin cannot. This is a dev dependency, so nothing shipped changes, and the upgrade produced no lint findings and no reformatting. - **Tooling — continuous integration.** The repo runs GitHub Actions on a pull request for the first time: `make check` and `make agent-test` on every one, a branch-flow guard carrying the workspace's closed prefix set, and version and changelog gates on a release-shaped pull request only. The shipped synthetic-inputs recipes run nightly, on demand, and on a pull request that touches their sources. No ruleset requires any of these checks, so they advise rather than block, and nothing runs on a merge. Nothing shipped changes; `docs/ci.md` is the account. +- **Tooling — `make check` refuses argument placeholders in a skill body.** Before the model reads a `SKILL.md`, Claude Code replaces `$ARGUMENTS` and `$0`, `$1`, … with the words the skill was invoked with, so a shell block containing one of those tokens reaches the model as a different command. The build now fails when any target's rendered `SKILL.md` contains one. No released skill contained one, so nothing shipped changes. ### Fixed diff --git a/docs/decisions.md b/docs/decisions.md index fbdd243e..6b14af73 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -223,6 +223,13 @@ The first real project through `pipelex-scaffold` acquired `pipelex-starter-js` - **`/pipelex-integrate` runs a project's bundle arm when it has one.** A project whose `make add-method` usage names a bundle path, as the method app's does, scaffolds a local bundle with that one command. The hand-written route stays for a project whose `make add-method` takes only a catalog id or an address. - **The blocks are executed, not only read.** `tests/unit/test_pipelex_scaffold_skill.py` runs the copy-out chain against a stand-in family repository, and the create and dev-server blocks against a `make` stand-in in every POSIX shell on the machine. That is how a `status=$?` was caught before it shipped: `status` is read-only in zsh, the default shell on macOS. +## Skill bodies carry no argument placeholders (2026-09-17) + +Claude Code rewrites a skill body before the model reads it. Read from the 2.1.273 bundle, the rule is this: `$ARGUMENTS` is always replaced, by nothing when the skill was invoked without arguments; `$ARGUMENTS[N]` and `$N` are replaced by the N-th whitespace-separated word of the arguments, counting from zero, when there are that many words, and are left as written otherwise; and a backslash before one of these tokens is removed, leaving the token as written. Reference files are not rewritten, because the model reads them with a tool. `pipelex-scaffold`'s dev-server block carried `"$1"` in its process-tree helper and `print $9` in the `awk` that read `lsof`'s address column. The second dogfood reading invoked the skill with a sentence as its arguments and received `kill -STOP "me"` and `print The`. As delivered, that block would have treated a server listening only on loopback as reachable from the network, stopped it and reported no URL. + +- **The fix removes the tokens instead of escaping them.** A backslash escape works in Claude Code, but Codex and Vibe show the template text unchanged, so their shells would read the backslash as part of the command. The process-tree helper now loops over its arguments with `local p; for p; do … done`. The `local` is required: without it, the helper's recursive calls reuse the same `p`, so each outer call signals the last process its recursive call visited instead of its own, which leaves every stopped parent suspended. The listener's address is now read with `lsof -Fn`, not by column number: `lsof` prints the socket state `(LISTEN)` after the address, so `$NF` would read the state and not the address. +- **`make check` fails when any target's rendered `SKILL.md` contains `$ARGUMENTS` or a `$` followed by a digit** (`check_skill_argument_placeholders` in `scripts/check.py`). The check is stricter than Claude Code: it also flags `$1x` and a `$N` that the invocation has too few words to fill, so it does not depend on either detail of the current rule. `${1}` is not substituted and does not fail the check. A skill that declares named arguments in its frontmatter would also have `$` substituted. No skill here declares any, so the first one that does must check its own body for those names. + ## License & distribution **Apache 2.0**; repo made public when ready (required for easy marketplace install). Versions start at **0.1.0** (plugin and marketplace). GitHub home assumed `Pipelex/pipelex-plugins` — confirm at first push. diff --git a/pipelex-codex/skills/pipelex-scaffold/SKILL.md b/pipelex-codex/skills/pipelex-scaffold/SKILL.md index 2d0166ac..2c168620 100644 --- a/pipelex-codex/skills/pipelex-scaffold/SKILL.md +++ b/pipelex-codex/skills/pipelex-scaffold/SKILL.md @@ -247,7 +247,7 @@ dir=$(cd && env pwd -P) || exit 1 log=$(mktemp "${TMPDIR:-/tmp}/pipelex-dev-XXXXXX") && page=$(mktemp "${TMPDIR:-/tmp}/pipelex-page-XXXXXX") || exit 1 nohup make -C "$dir" dev APP_PORT= APP_HOST=127.0.0.1 > "$log" 2>&1 & launcher=$! -stop_tree() { kill -STOP "$1" 2>/dev/null || return 0; for child in $(pgrep -P "$1"); do stop_tree "$child"; done; kill -TERM "$1" 2>/dev/null; kill -CONT "$1" 2>/dev/null; } +stop_tree() { local p; for p; do kill -STOP "$p" 2>/dev/null || continue; stop_tree $(pgrep -P "$p"); kill -TERM "$p" 2>/dev/null; kill -CONT "$p" 2>/dev/null; done; } n=0; until lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1 || ! kill -0 "$launcher" 2>/dev/null || [ "$n" -ge 300 ]; do sleep 0.2; n=$((n + 1)); done if ! lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1; then if kill -0 "$launcher" 2>/dev/null; then stop_tree "$launcher"; echo "nothing listens on port yet, so the server this command started was stopped; server log: $log" >&2; exit 1; fi @@ -255,7 +255,7 @@ if ! lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1; then fi for pid in $(lsof -ti tcp: -sTCP:LISTEN); do [ "$(lsof -a -p "$pid" -d cwd -Fn 2>/dev/null | sed -n 's/^n//p' | head -n 1)" = "$dir" ] || { echo "port is held by pid $pid, which is not this project; server log: $log" >&2; exit 1; } - if lsof -nP -a -p "$pid" -iTCP: -sTCP:LISTEN | awk 'NR > 1 { print $9 }' | grep -Evq '^(127\.0\.0\.1|\[::1\]):$'; then + if lsof -nP -a -p "$pid" -iTCP: -sTCP:LISTEN -Fn | sed -n 's/^n//p' | grep -Evq '^(127\.0\.0\.1|\[::1\]):$'; then kill "$pid"; echo "the server listened beyond this machine and was stopped; server log: $log" >&2; exit 1 fi done diff --git a/pipelex-vibe/skills/pipelex-scaffold/SKILL.md b/pipelex-vibe/skills/pipelex-scaffold/SKILL.md index 72422257..40d94c3a 100644 --- a/pipelex-vibe/skills/pipelex-scaffold/SKILL.md +++ b/pipelex-vibe/skills/pipelex-scaffold/SKILL.md @@ -247,7 +247,7 @@ dir=$(cd && env pwd -P) || exit 1 log=$(mktemp "${TMPDIR:-/tmp}/pipelex-dev-XXXXXX") && page=$(mktemp "${TMPDIR:-/tmp}/pipelex-page-XXXXXX") || exit 1 nohup make -C "$dir" dev APP_PORT= APP_HOST=127.0.0.1 > "$log" 2>&1 & launcher=$! -stop_tree() { kill -STOP "$1" 2>/dev/null || return 0; for child in $(pgrep -P "$1"); do stop_tree "$child"; done; kill -TERM "$1" 2>/dev/null; kill -CONT "$1" 2>/dev/null; } +stop_tree() { local p; for p; do kill -STOP "$p" 2>/dev/null || continue; stop_tree $(pgrep -P "$p"); kill -TERM "$p" 2>/dev/null; kill -CONT "$p" 2>/dev/null; done; } n=0; until lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1 || ! kill -0 "$launcher" 2>/dev/null || [ "$n" -ge 300 ]; do sleep 0.2; n=$((n + 1)); done if ! lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1; then if kill -0 "$launcher" 2>/dev/null; then stop_tree "$launcher"; echo "nothing listens on port yet, so the server this command started was stopped; server log: $log" >&2; exit 1; fi @@ -255,7 +255,7 @@ if ! lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1; then fi for pid in $(lsof -ti tcp: -sTCP:LISTEN); do [ "$(lsof -a -p "$pid" -d cwd -Fn 2>/dev/null | sed -n 's/^n//p' | head -n 1)" = "$dir" ] || { echo "port is held by pid $pid, which is not this project; server log: $log" >&2; exit 1; } - if lsof -nP -a -p "$pid" -iTCP: -sTCP:LISTEN | awk 'NR > 1 { print $9 }' | grep -Evq '^(127\.0\.0\.1|\[::1\]):$'; then + if lsof -nP -a -p "$pid" -iTCP: -sTCP:LISTEN -Fn | sed -n 's/^n//p' | grep -Evq '^(127\.0\.0\.1|\[::1\]):$'; then kill "$pid"; echo "the server listened beyond this machine and was stopped; server log: $log" >&2; exit 1 fi done diff --git a/pipelex/skills/pipelex-scaffold/SKILL.md b/pipelex/skills/pipelex-scaffold/SKILL.md index 63ed0ac9..feae9cca 100644 --- a/pipelex/skills/pipelex-scaffold/SKILL.md +++ b/pipelex/skills/pipelex-scaffold/SKILL.md @@ -254,7 +254,7 @@ dir=$(cd && env pwd -P) || exit 1 log=$(mktemp "${TMPDIR:-/tmp}/pipelex-dev-XXXXXX") && page=$(mktemp "${TMPDIR:-/tmp}/pipelex-page-XXXXXX") || exit 1 nohup make -C "$dir" dev APP_PORT= APP_HOST=127.0.0.1 > "$log" 2>&1 & launcher=$! -stop_tree() { kill -STOP "$1" 2>/dev/null || return 0; for child in $(pgrep -P "$1"); do stop_tree "$child"; done; kill -TERM "$1" 2>/dev/null; kill -CONT "$1" 2>/dev/null; } +stop_tree() { local p; for p; do kill -STOP "$p" 2>/dev/null || continue; stop_tree $(pgrep -P "$p"); kill -TERM "$p" 2>/dev/null; kill -CONT "$p" 2>/dev/null; done; } n=0; until lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1 || ! kill -0 "$launcher" 2>/dev/null || [ "$n" -ge 300 ]; do sleep 0.2; n=$((n + 1)); done if ! lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1; then if kill -0 "$launcher" 2>/dev/null; then stop_tree "$launcher"; echo "nothing listens on port yet, so the server this command started was stopped; server log: $log" >&2; exit 1; fi @@ -262,7 +262,7 @@ if ! lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1; then fi for pid in $(lsof -ti tcp: -sTCP:LISTEN); do [ "$(lsof -a -p "$pid" -d cwd -Fn 2>/dev/null | sed -n 's/^n//p' | head -n 1)" = "$dir" ] || { echo "port is held by pid $pid, which is not this project; server log: $log" >&2; exit 1; } - if lsof -nP -a -p "$pid" -iTCP: -sTCP:LISTEN | awk 'NR > 1 { print $9 }' | grep -Evq '^(127\.0\.0\.1|\[::1\]):$'; then + if lsof -nP -a -p "$pid" -iTCP: -sTCP:LISTEN -Fn | sed -n 's/^n//p' | grep -Evq '^(127\.0\.0\.1|\[::1\]):$'; then kill "$pid"; echo "the server listened beyond this machine and was stopped; server log: $log" >&2; exit 1 fi done diff --git a/scripts/check.py b/scripts/check.py index 0f198883..13cee1a1 100644 --- a/scripts/check.py +++ b/scripts/check.py @@ -16,6 +16,8 @@ SHARED_TEMPLATE_FILES = [Path(template_path).name for template_path in SHARED_TEMPLATES] _SHARED_STEMS = [Path(template_path).name.removesuffix(".md.j2") for template_path in SHARED_TEMPLATES] STALE_REF_PATTERN = re.compile(r"references/(?:" + "|".join(re.escape(stem) for stem in _SHARED_STEMS) + r")") +# Claude Code replaces `$ARGUMENTS`, `$ARGUMENTS[N]` and `$N` in a skill body with the invocation's arguments. +ARGUMENT_PLACEHOLDER_PATTERN = re.compile(r"\$(?:ARGUMENTS|\d+)") TARGETS_DIR_NAME = "targets" DEFAULTS_FILE = "defaults.toml" @@ -369,6 +371,26 @@ def check_stale_references(base_dir: Path) -> list[str]: return errors +def check_skill_argument_placeholders(base_dir: Path) -> list[str]: + """Check that no generated SKILL.md carries a token Claude Code replaces with the invocation's arguments. + + Before the model reads a skill body, Claude Code substitutes `$ARGUMENTS` with the whole argument + string and `$0`, `$1`, … with its whitespace-separated words, so shell code holding `"$1"` or + `awk '{ print $9 }'` reaches the model as a different command whenever the skill was invoked with + enough words. The pattern is wider than Claude Code's own, which spares `$1x` and leaves `$N` alone + when there are too few words: a guard should not depend on either detail. Every target is scanned + because the body is shared, and reference files are not, because a tool reads them unsubstituted. + """ + errors: list[str] = [] + for output_dir in _collect_output_dirs(base_dir): + for skill_md in sorted(output_dir.glob("skills/*/SKILL.md")): + for idx, line in enumerate(skill_md.read_text(encoding="utf-8").splitlines(), start=1): + for match in ARGUMENT_PLACEHOLDER_PATTERN.finditer(line): + rel = skill_md.relative_to(base_dir) + errors.append(f"{rel}:{idx}: `{match.group()}` is replaced by the skill's invocation arguments in Claude Code") + return errors + + def check_shared_files_exist(base_dir: Path) -> list[str]: """Check that all expected shared template source files are present.""" shared_dir = base_dir / "templates" / "skills" / "shared" @@ -558,6 +580,12 @@ def run_shared_checks(base_dir: Path) -> bool: "FAIL: Found stale references/ paths (should use ../shared/ instead).", " No stale references found.", ) + failed |= _run_check( + "Checking skill bodies for tokens Claude Code replaces with invocation arguments...", + check_skill_argument_placeholders(base_dir), + "FAIL: Found $ARGUMENTS or $ in a SKILL.md. Rewrite without the token (an escape reaches Codex and Vibe verbatim).", + " No argument placeholders found.", + ) failed |= _run_check( "Checking all shared template files exist...", check_shared_files_exist(base_dir), diff --git a/templates/skills/pipelex-scaffold/SKILL.md.j2 b/templates/skills/pipelex-scaffold/SKILL.md.j2 index 64599760..0e48dcef 100644 --- a/templates/skills/pipelex-scaffold/SKILL.md.j2 +++ b/templates/skills/pipelex-scaffold/SKILL.md.j2 @@ -247,7 +247,7 @@ dir=$(cd && env pwd -P) || exit 1 log=$(mktemp "${TMPDIR:-/tmp}/pipelex-dev-XXXXXX") && page=$(mktemp "${TMPDIR:-/tmp}/pipelex-page-XXXXXX") || exit 1 nohup make -C "$dir" dev APP_PORT= APP_HOST=127.0.0.1 > "$log" 2>&1 & launcher=$! -stop_tree() { kill -STOP "$1" 2>/dev/null || return 0; for child in $(pgrep -P "$1"); do stop_tree "$child"; done; kill -TERM "$1" 2>/dev/null; kill -CONT "$1" 2>/dev/null; } +stop_tree() { local p; for p; do kill -STOP "$p" 2>/dev/null || continue; stop_tree $(pgrep -P "$p"); kill -TERM "$p" 2>/dev/null; kill -CONT "$p" 2>/dev/null; done; } n=0; until lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1 || ! kill -0 "$launcher" 2>/dev/null || [ "$n" -ge 300 ]; do sleep 0.2; n=$((n + 1)); done if ! lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1; then if kill -0 "$launcher" 2>/dev/null; then stop_tree "$launcher"; echo "nothing listens on port yet, so the server this command started was stopped; server log: $log" >&2; exit 1; fi @@ -255,7 +255,7 @@ if ! lsof -ti tcp: -sTCP:LISTEN > /dev/null 2>&1; then fi for pid in $(lsof -ti tcp: -sTCP:LISTEN); do [ "$(lsof -a -p "$pid" -d cwd -Fn 2>/dev/null | sed -n 's/^n//p' | head -n 1)" = "$dir" ] || { echo "port is held by pid $pid, which is not this project; server log: $log" >&2; exit 1; } - if lsof -nP -a -p "$pid" -iTCP: -sTCP:LISTEN | awk 'NR > 1 { print $9 }' | grep -Evq '^(127\.0\.0\.1|\[::1\]):$'; then + if lsof -nP -a -p "$pid" -iTCP: -sTCP:LISTEN -Fn | sed -n 's/^n//p' | grep -Evq '^(127\.0\.0\.1|\[::1\]):$'; then kill "$pid"; echo "the server listened beyond this machine and was stopped; server log: $log" >&2; exit 1 fi done diff --git a/tests/unit/test_check.py b/tests/unit/test_check.py index 3ef9100d..c59df6a5 100644 --- a/tests/unit/test_check.py +++ b/tests/unit/test_check.py @@ -14,6 +14,7 @@ check_matched_target_versions, check_no_templates_in_output, check_shared_files_exist, + check_skill_argument_placeholders, check_stale_references, check_target_plugin_versions, check_vibe_target_artifacts, @@ -457,6 +458,73 @@ def test_ignores_correct_shared_path(self, skill_tree: Path) -> None: assert check_stale_references(skill_tree) == [] +class TestSkillArgumentPlaceholders: + def test_clean_tree(self, skill_tree: Path) -> None: + assert check_skill_argument_placeholders(skill_tree) == [] + + @pytest.mark.parametrize( + ("line", "token"), + [ + ('stop_tree() { kill -STOP "$1" 2>/dev/null; }', "$1"), + ("awk 'NR > 1 { print $9 }'", "$9"), + ('echo "$0"', "$0"), + ('set -- "$10"', "$10"), + ("Summarize $ARGUMENTS.", "$ARGUMENTS"), + ("Open $ARGUMENTS[0] first.", "$ARGUMENTS"), + (r'kill "\$1"', "$1"), + ('echo "$1x"', "$1"), + ], + ids=["quoted", "awk-field", "zero", "two-digits", "arguments", "indexed", "escaped", "word-suffix"], + ) + def test_detects_placeholder(self, skill_tree: Path, line: str, token: str) -> None: + skill_md = skill_tree / "pipelex" / "skills" / "pipelex-test" / "SKILL.md" + skill_md.write_text(VALID_FRONTMATTER + f"\n```bash\n{line}\n```\n") + errors = check_skill_argument_placeholders(skill_tree) + assert len(errors) == 1 + assert errors[0].startswith("pipelex/skills/pipelex-test/SKILL.md:9: ") + assert f"`{token}`" in errors[0] + + def test_reports_every_token_on_a_line(self, skill_tree: Path) -> None: + skill_md = skill_tree / "pipelex" / "skills" / "pipelex-test" / "SKILL.md" + skill_md.write_text(VALID_FRONTMATTER + '\nfor child in $(pgrep -P "$1"); do stop_tree "$2"; done\n') + errors = check_skill_argument_placeholders(skill_tree) + assert [error.split("`")[1] for error in errors] == ["$1", "$2"] + + def test_ignores_shell_tokens_claude_code_leaves_alone(self, skill_tree: Path) -> None: + skill_md = skill_tree / "pipelex" / "skills" / "pipelex-test" / "SKILL.md" + skill_md.write_text( + VALID_FRONTMATTER + + "\n```bash\n" + + 'launcher=$!; echo $$ "$?" "$pid" "${APP_PORT:-4300}" "${5#APP_HOST=}" "$((n + 1))" "$(pwd)"\n' + + 'stop_tree() { local p; for p; do kill -STOP "$p"; done; }\n' + + "```\n" + ) + assert check_skill_argument_placeholders(skill_tree) == [] + + def test_ignores_reference_files(self, skill_tree: Path) -> None: + """A tool reads references and shared files as they are, so Claude Code substitutes nothing in them.""" + references = skill_tree / "pipelex" / "skills" / "pipelex-test" / "references" + references.mkdir() + (references / "recipes.md").write_text("Dollar amounts (`$100`) and `print $9`.\n") + (skill_tree / "pipelex" / "skills" / "shared" / "mthds-reference.md").write_text("Dollar amounts (`$100`).\n") + assert check_skill_argument_placeholders(skill_tree) == [] + + def test_scans_every_target(self, skill_tree: Path) -> None: + _write_target_configs( + skill_tree, + { + "prod": {"name": "pipelex", "version": "0.6.3", "source": "pipelex/"}, + "codex": {"name": "pipelex", "version": "0.6.3", "source": "pipelex-codex/"}, + }, + ) + codex_skill = skill_tree / "pipelex-codex" / "skills" / "pipelex-test" + codex_skill.mkdir(parents=True) + (codex_skill / "SKILL.md").write_text(VALID_FRONTMATTER + "\nRun it with $ARGUMENTS.\n") + errors = check_skill_argument_placeholders(skill_tree) + assert len(errors) == 1 + assert errors[0].startswith("pipelex-codex/skills/pipelex-test/SKILL.md:") + + class TestSharedFilesExist: def test_all_present(self, skill_tree: Path) -> None: assert check_shared_files_exist(skill_tree) == [] From 85657ba9d1fee7d40a5d222a940e63dafc2f63c7 Mon Sep 17 00:00:00 2001 From: Louis Choquel Date: Thu, 17 Sep 2026 09:27:32 +0200 Subject: [PATCH 2/2] Relay make create's warnings and clean up an interrupted clone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scaffold's create block showed only the last lines of make create's log, which are make all's, so the warnings the gesture prints earlier never reached the report: a created MIT project kept the template's copyright line and nobody was told. The block now lists each distinct warning from the whole log, and the report relays each with what answers it, the LICENSE holder first. Both acquisition chains now remove their temporary clone with one trap set right after mktemp, trapping INT and TERM as well as EXIT, because zsh and dash run no EXIT trap when a signal kills them. A session cleared or a command stopped mid-clone no longer leaves a .pipelex-method-apps-… or .pipelex-starter-… directory beside the project. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013DQGXP7o1q5tm6LJLfSsRE --- CHANGELOG.md | 2 +- docs/decisions.md | 7 + .../skills/pipelex-scaffold/SKILL.md | 35 +++-- .../pipelex-scaffold/references/starters.md | 24 ++-- pipelex-vibe/skills/pipelex-scaffold/SKILL.md | 35 +++-- .../pipelex-scaffold/references/starters.md | 24 ++-- pipelex/skills/pipelex-scaffold/SKILL.md | 35 +++-- .../pipelex-scaffold/references/starters.md | 24 ++-- .../pipelex-scaffold/references/starters.md | 24 ++-- templates/skills/pipelex-scaffold/SKILL.md.j2 | 35 +++-- tests/unit/test_pipelex_scaffold_skill.py | 136 ++++++++++++++++++ 11 files changed, 276 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 787f0688..4632c1e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Added - **`pipelex-integrate` — wire an MTHDS method into a Python or TypeScript codebase.** Given a local bundle, a published `method_ref` at a tag, or a catalog `method_id`, and a project, the skill picks the codegen target by audience (`ts-zod`; `python-pydantic` for a hosted-API consumer, `python-structures` for a Pipelex host), has the workshop's `mthds_codegen` write the generated tree into one dedicated directory per method through its `output_dir` arm — no artifact byte ever passes through the model, and a refused write is never worked around by writing bytes from the conversation — excludes that tree, and the drift-gate script it copies into the project, from the project's formatters and linters *before* either exists while keeping the type checker's coverage as it is, records a `sources.json` sidecar (selector, target, pipe signature, the bundle directory the call site loads, and a hash for every `.mthds` file under it) so a second run is a refresh and a bundle change — a file edited, removed or added — is detectable, wires an offline drift gate into the project's existing check — `scripts/codegen-check.mjs` over `@pipelex/sdk`'s `runCodegenCheck` for TypeScript, `scripts/codegen_check.py` over `pipelex-sdk`'s `run_codegen_check` (run in the project's own environment) for a Python consumer, each on an SDK raised to the skill's floor when the project pins an older one (`@pipelex/sdk` 0.17.0, `pipelex-sdk` 0.10.0), neither adding the `pipelex` runtime and neither reading its own failure to run as drift: an SDK that cannot be imported or fails while it loads, or a check that throws, exits `2`, no verdict — and writes one typed call-site module per method over `startAndWaitForResult` / `start_and_wait` and the generated binder or model. The call site is typed from the main pipe's signature on the validate verdict. A refresh keeps the gate current as well: it verifies the tooling exclusions first, installs a gate script the project lacks and wires it, re-copies one that differs from the shipped reference — which, kept out of the formatters and linters, only an out-of-date script does — and adds the refreshed method's directory to the gate command when it is missing, leaving every other method's registration as it is. A project made from a Pipelex template keeps its own codegen harness: the skill runs the project's `codegen` script or `make add-method`, which scaffolds a local bundle in one command on a project whose `make add-method` takes a bundle path, and never writes a second layout beside the first. Two things it refuses outright, because both are silent: generating a second method into a directory that already holds another method's tree (every method of a target emits the same file names, so that overwrites the first and reports no orphan), and reaching a project the workshop was not launched in (a path inside the workshop is legal wherever it points, so containment is read before the first write and a tree that landed beside the wrong project is never moved across). A write whose only fault is orphans is not one of them: stamped files the new lock does not list make the drift check report the tree non-current, but with nothing else drifted the generation itself is sound, so the skill reads `orphans[]` and `drifts[]` rather than the verdict, finishes the integration, and reports the orphan paths by name — never deleted, with a dedicated directory per generation as the fix and the gate counting each orphan as a drift until the directory holds one generation. Any other drift ends the run. One upstream defect it names rather than patches: the `ts-zod` emitter writes `binder.ts`'s sibling import without a file extension, which a plain Node ESM project rejects at type-check and at runtime while a bundler resolution accepts — the generated tree is stamped and hashed, so the fix is the emitter's and the skill says so. Ships `references/typescript.md`, `references/python.md`, `references/codegen-check.mjs` and `references/codegen_check.py`. -- **`pipelex-scaffold` — the front door to a project that does not exist yet.** Two branches and no templates of its own: a Pipelex template, or the ecosystem's initializer (`uv init --package`, `npm create next-app@latest`, …) when the user wants their framework. A TypeScript web app around a method is the demo-free method app, the `webapp-js/` directory of `pipelex-method-apps`: the skill copies that directory out of a shallow clone into the destination, commits it once as it came, runs the template's **own** `make create` with the user's bundle, catalog id or package address, which names the project after the method, scaffolds its form and result view, writes `.env.local` and runs `make all`, and then starts the dev server on a free port and reports its URL first, once one request has proven the page answers. The page's Server Actions spend the key for whoever reaches them, so the server is started only from a copy whose dev script binds it to loopback, and the start command stops it before the first request if `lsof` shows it listening anywhere else, or if its port has not opened by the end of the wait. A copy already in the working directory is checked before git is initialized in it, so the template's own checkout, which sits inside the family repository, is never taken for a copy. The `pipelex-starter-js` gallery is acquired only when the user names it. A starter — the gallery, or `pipelex-starter-python` for a CLI or service — is acquired as a fresh-history local clone by default or through `gh repo create --template` after confirmation, committed once as it came, then renamed by the clone's **own** `bootstrap` skill, read from its `SKILL.md` and never reimplemented. The starters and the initializer end with the env-file convention, the key filled only from the shell environment and never asked for in the conversation, the base URL copied from the environment whenever the shell sets one, with a key or without — a key is refused by every plane but the one that issued it and a keyless self-hosted runner is a plane too, so neither a dev or staging key nor a self-hosted project is left pointing at production's URL — with the report naming the plane the file points at and warning, when it is not production, that a key from `app.pipelex.com` will be refused there, and a hand-off to `/pipelex-integrate`. It is the plugin's third MCP-free skill, and never handles a key itself: the method app's gesture reads the key from the shell or from an `.env.local` the user writes. Ships `references/starters.md`, which sets the method app, the gallery and the Python starter side by side, and `references/initializers.md`. +- **`pipelex-scaffold` — the front door to a project that does not exist yet.** Two branches and no templates of its own: a Pipelex template, or the ecosystem's initializer (`uv init --package`, `npm create next-app@latest`, …) when the user wants their framework. A TypeScript web app around a method is the demo-free method app, the `webapp-js/` directory of `pipelex-method-apps`: the skill copies that directory out of a shallow clone into the destination, commits it once as it came, runs the template's **own** `make create` with the user's bundle, catalog id or package address, which names the project after the method, scaffolds its form and result view, writes `.env.local` and runs `make all`, and then starts the dev server on a free port and reports its URL first, once one request has proven the page answers. The report relays every warning the gesture printed, starting with an MIT `LICENSE` that still names the template's copyright holder, and a clone interrupted by Ctrl-C or by the harness leaves no temporary directory beside the project. The page's Server Actions spend the key for whoever reaches them, so the server is started only from a copy whose dev script binds it to loopback, and the start command stops it before the first request if `lsof` shows it listening anywhere else, or if its port has not opened by the end of the wait. A copy already in the working directory is checked before git is initialized in it, so the template's own checkout, which sits inside the family repository, is never taken for a copy. The `pipelex-starter-js` gallery is acquired only when the user names it. A starter — the gallery, or `pipelex-starter-python` for a CLI or service — is acquired as a fresh-history local clone by default or through `gh repo create --template` after confirmation, committed once as it came, then renamed by the clone's **own** `bootstrap` skill, read from its `SKILL.md` and never reimplemented. The starters and the initializer end with the env-file convention, the key filled only from the shell environment and never asked for in the conversation, the base URL copied from the environment whenever the shell sets one, with a key or without — a key is refused by every plane but the one that issued it and a keyless self-hosted runner is a plane too, so neither a dev or staging key nor a self-hosted project is left pointing at production's URL — with the report naming the plane the file points at and warning, when it is not production, that a key from `app.pipelex.com` will be refused there, and a hand-off to `/pipelex-integrate`. It is the plugin's third MCP-free skill, and never handles a key itself: the method app's gesture reads the key from the shell or from an `.env.local` the user writes. Ships `references/starters.md`, which sets the method app, the gallery and the Python starter side by side, and `references/initializers.md`. - **`pipelex-vibe/mcp/vibe-mcp.toml` — the Mistral Vibe target bakes the workshop launcher**: Vibe has no plugin manifest, so the target now ships the `npx -y @pipelex/mcp@latest` launcher as a `[[mcp_servers]]` stdio entry to append to the end of `~/.vibe/config.toml`, rendered from the same `[vars.mcp_server]` block as the Claude and Codex manifests and enforced by `make check`. Write your API key into the entry's `env` table, because Vibe spawns stdio servers with a minimal environment and never sees an exported `PIPELEX_API_KEY`; the MCP-backed skills' Vibe stop message now points at the fragment and says so, and their Vibe auth line no longer claims the server reads the session environment. Before appending, delete the `mcp_servers = []` line a new Vibe config carries and any `pipelex` server registered by hand: either leftover stops Vibe from starting. Vibe records its configuration, that key included, in every session log under `~/.vibe/logs/session/`, so redact the key before sharing one. - **`pipelex-synthetic-inputs` — a skill that renders the files a method needs, from code.** PDFs through `reportlab` (canvas letters, multi-page Platypus reports, tables, and a composed line-item document whose totals come from its items) and PNGs through `Pillow` and `matplotlib` in four categories: `chart` (bar, line, pie, scatter), `diagram` (a node/edge list laid out on a grid with clipped arrows), `document_scan` (an A4-at-150-dpi page put through a seeded skew/tint/grain/vignette post-process, for OCR and document-understanding methods) and `screenshot` (window chrome, sidebar, stat tiles, and a status-badged table or card grid). Word and Excel come along from `pipelex-inputs`. No AI is involved anywhere, and only packages whose licences are compatible with MIT are used. **Photographs and handwriting are deliberately out of scope** — code cannot render either to a standard a vision model would accept, so the skill says so and asks for a real file instead of handing a method an imitation. It is MCP-free, the second such skill after `pipelex-explain`, and it installs what it needs itself: `uv` with ephemeral packages, or a venv it creates under the user's cache directory when `uv` is absent. Nothing is installed into the project, and installing a *tool* always asks first. - **The recipes refuse to render a file that would be silently wrong.** A recipe is copied and adapted, so the content block is where things go wrong: each one now bounds its content against the page it is drawing on and stops with the cure rather than exiting 0 on a file that lies. A `document_scan` whose items overrun the page, a `screenshot` whose rows run off the canvas, a `diagram` with two nodes in one grid cell, an unsubstituted `` still in the path — every one of these used to print success and hand a method an input missing exactly the field it was meant to read. Line-item descriptions wrap instead of overprinting the quantity beside them, the PDF table recipe sizes its columns instead of drawing them off the paper, and every recipe renders beside its target and renames on success, so a crash can no longer truncate a file the user already had. diff --git a/docs/decisions.md b/docs/decisions.md index 6b14af73..e57a58b4 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -230,6 +230,13 @@ Claude Code rewrites a skill body before the model reads it. Read from the 2.1.2 - **The fix removes the tokens instead of escaping them.** A backslash escape works in Claude Code, but Codex and Vibe show the template text unchanged, so their shells would read the backslash as part of the command. The process-tree helper now loops over its arguments with `local p; for p; do … done`. The `local` is required: without it, the helper's recursive calls reuse the same `p`, so each outer call signals the last process its recursive call visited instead of its own, which leaves every stopped parent suspended. The listener's address is now read with `lsof -Fn`, not by column number: `lsof` prints the socket state `(LISTEN)` after the address, so `$NF` would read the state and not the address. - **`make check` fails when any target's rendered `SKILL.md` contains `$ARGUMENTS` or a `$` followed by a digit** (`check_skill_argument_placeholders` in `scripts/check.py`). The check is stricter than Claude Code: it also flags `$1x` and a `$N` that the invocation has too few words to fill, so it does not depend on either detail of the current rule. `${1}` is not substituted and does not fail the check. A skill that declares named arguments in its frontmatter would also have `$` substituted. No skill here declares any, so the first one that does must check its own body for those names. +## The scaffold relays the gesture's warnings and cleans up when interrupted (2026-09-17) + +The second dogfood reading found two more gaps in the method app's path. + +- **The create block lists every warning `make create` printed, and the report relays each one.** The gesture prints its own warnings as `! …` and the bootstrap's as `warning: …`, all before `make all`, whose output fills the 40-line tail the block showed. So the project the reading created kept the template owner's copyright line in its MIT `LICENSE`, and the report never said so. The block now reads the warnings from the whole log. It dedupes them in byte order (`LC_ALL=C sort -u`), because the bootstrap runs twice and prints each warning twice, and because some locales' collation could merge two warnings that differ only in punctuation. The report relays each warning with what answers it, and the `LICENSE` holder comes first. The gesture refuses to run on a project it has already made, so after the run the answer is an edit of `LICENSE`. In interactive mode the dry run shows the warning first, and `LICENSE_HOLDER` goes into the real run. +- **Both acquisition chains clean up with one trap set on the line after `mktemp`: `trap 'rm -rf "$tmp"' EXIT; trap 'exit 130' INT; trap 'exit 143' TERM`.** The reading found an empty `.pipelex-method-apps-…` beside an empty project, left by a session cleared while the chain ran, and the chain removed its temporary path only on the failures it expected. Claude Code stops a command by sending `TERM` to its process group and `KILL` 1.5 seconds later (read from the 2.1.273 bundle), and Ctrl-C sends `INT`. Removing a partial shallow clone takes far less than 1.5 seconds, so the trap has time to run. The `INT` and `TERM` traps are required: bash runs an `EXIT` trap when a signal kills it, but zsh and dash do not, and without those two traps both shells left the directory behind. `tests/unit/test_pipelex_scaffold_skill.py` interrupts both chains mid-clone in every shell on the machine. A `KILL` still leaves the directory behind, and the failure table tells the next run to leave such a directory alone and name it in the report. + ## License & distribution **Apache 2.0**; repo made public when ready (required for easy marketplace install). Versions start at **0.1.0** (plugin and marketplace). GitHub home assumed `Pipelex/pipelex-plugins` — confirm at first push. diff --git a/pipelex-codex/skills/pipelex-scaffold/SKILL.md b/pipelex-codex/skills/pipelex-scaffold/SKILL.md index 2c168620..afb6c30f 100644 --- a/pipelex-codex/skills/pipelex-scaffold/SKILL.md +++ b/pipelex-codex/skills/pipelex-scaffold/SKILL.md @@ -89,17 +89,17 @@ The template is one directory of the `pipelex-method-apps` repository, and a pro mkdir -p && dir=$(cd && pwd) || exit 1 case "$(ls -A "$dir")" in ""|.git) ;; *) exit 1 ;; esac tmp=$(mktemp -d "$(dirname "$dir")/.pipelex-method-apps-XXXXXX") || exit 1 -git clone --depth 1 https://github.com/Pipelex/pipelex-method-apps.git "$tmp" || { rm -rf "$tmp"; exit 1; } +trap 'rm -rf "$tmp"' EXIT; trap 'exit 130' INT; trap 'exit 143' TERM +git clone --depth 1 https://github.com/Pipelex/pipelex-method-apps.git "$tmp" || exit 1 git -C "$tmp" rev-parse HEAD # the family SHA, for the commit message cat "$tmp/VERSION" # the family version, for the commit message -[ -f "$tmp/webapp-js/package.json" ] || { echo "the default branch carries no webapp-js/" >&2; rm -rf "$tmp"; exit 1; } -case "$(ls -A "$dir")" in ""|.git) ;; *) rm -rf "$tmp"; exit 1 ;; esac -cp -R "$tmp/webapp-js"/. "$dir"/ || { rm -rf "$tmp"; exit 1; } -rm -rf "$tmp" +[ -f "$tmp/webapp-js/package.json" ] || { echo "the default branch carries no webapp-js/" >&2; exit 1; } +case "$(ls -A "$dir")" in ""|.git) ;; *) exit 1 ;; esac +cp -R "$tmp/webapp-js"/. "$dir"/ || exit 1 [ -e "$dir/.git" ] || git -C "$dir" init -b main ``` -It is the starters' acquisition beside the directory, below, and the properties that make that one safe hold here for the reasons given there. The destination is resolved before its parent is taken, so a `` spelled `.` keeps the temporary path a sibling. No `rm -rf` addresses a path under ``. `cp -R "$tmp/webapp-js"/. "$dir"/` carries the entries beginning with a dot (`.gitignore`, `.env.example`, `.claude/`, `.husky/`). The directory is read once before anything is fetched and once more right before the copy, and anything but nothing or a lone `.git` stops the run with nothing copied and the temporary path removed. The chain goes out as one command. **The last line initializes only a directory with no repository of its own**, so a repository the user made goes on standing, and the pristine commit lands on their branch, as Step 3 says. +It is the starters' acquisition beside the directory, below, and the properties that make that one safe hold here for the reasons given there. The destination is resolved before its parent is taken, so a `` spelled `.` keeps the temporary path a sibling. One trap removes the temporary path however the command ends, an interruption included. No `rm -rf` addresses a path under ``. `cp -R "$tmp/webapp-js"/. "$dir"/` carries the entries beginning with a dot (`.gitignore`, `.env.example`, `.claude/`, `.husky/`). The directory is read once before anything is fetched and once more right before the copy, and anything but nothing or a lone `.git` stops the run with nothing copied and the temporary path removed. The chain goes out as one command. **The last line initializes only a directory with no repository of its own**, so a repository the user made goes on standing, and the pristine commit lands on their branch, as Step 3 says. **The `webapp-js/` test is load-bearing.** The copy takes the default branch's head, and a head that does not carry the directory would otherwise copy nothing and report success. Stop there, say what the clone lacked, and do not fall back to the gallery or to another directory of the repository. @@ -123,13 +123,13 @@ The clone's `.git` is removed on purpose: it is the template's history and remot ```bash dir=$(cd && pwd) || exit 1 tmp=$(mktemp -d "$(dirname "$dir")/.pipelex-starter-XXXXXX") || exit 1 -git clone --depth 1 https://github.com/Pipelex/.git "$tmp" || { rm -rf "$tmp"; exit 1; } +trap 'rm -rf "$tmp"' EXIT; trap 'exit 130' INT; trap 'exit 143' TERM +git clone --depth 1 https://github.com/Pipelex/.git "$tmp" || exit 1 git -C "$tmp" rev-parse HEAD # the template SHA, for the commit message # the template version: package.json "version" (JS) or pyproject.toml version (Python) -rm -rf "$tmp/.git" || { rm -rf "$tmp"; exit 1; } -[ "$(ls -A "$dir")" = ".git" ] || { rm -rf "$tmp"; exit 1; } -cp -R "$tmp"/. "$dir"/ || { rm -rf "$tmp"; exit 1; } -rm -rf "$tmp" +rm -rf "$tmp/.git" || exit 1 +[ "$(ls -A "$dir")" = ".git" ] || exit 1 +cp -R "$tmp"/. "$dir"/ || exit 1 ``` **The first line resolves the destination, and that is what keeps the temporary path a sibling rather than a child.** `` is very often `.` here: `mkdir my-app && cd my-app && git init` is the "Where" rule's own account of how a user arrives at a directory holding nothing but `.git`, and they then ask for the project *here*. `dirname .` is `.`, so deriving the parent from the spelling would put the temporary directory **inside** the destination, where the `ls -A` line below finds it sitting beside `.git` and refuses — every time, on exactly the case this section exists to serve. Resolving to an absolute path first also pins the destination for the rest of the chain, so no later line can be re-read against a working directory that has moved, and it is what lets every mention below be quoted: a name with a space reaches `cp` whole instead of arriving as two arguments. @@ -140,7 +140,9 @@ rm -rf "$tmp" **The `ls -A` line is the "Where" rule read again, against the copy.** It is not the decision — the "Where" question settled that — it is the last look before anything lands, put next to the copy so nothing can change between the two. It admits exactly one entry, `.git`, which the clone has not had since the line above: a collision is therefore impossible rather than merely unlikely, and the template can only add to the directory. Anything else — `.git` beside a file of the user's, a `.DS_Store`, a `README.md` they wrote — stops here with nothing copied, the temporary path removed and the directory as it was. A discarded shallow clone is the cheap half of that trade. Nothing of the user's is overwritten, moved or deleted to make room, here or anywhere. -**The chain goes out as one command.** The guards hold only inside one shell — the same reason the `|| exit` above is load-bearing, stated in full in [references/starters.md](references/starters.md) — and split across separate calls this one loses its cleanup too, leaving the temporary directory beside the user's project with no line left to remove it. +**The chain goes out as one command.** The guards hold only inside one shell — the same reason the `|| exit` above is load-bearing, stated in full in [references/starters.md](references/starters.md) — and split across separate calls this one loses its cleanup too, because a trap lasts only as long as the shell that set it. + +**One trap removes the temporary path, however the command ends.** It is set on the line after `mktemp`, before anything can fail, so a failed clone, a refusal, a success and an interrupted command all end the same way. The `INT` and `TERM` traps cover the interruption: Ctrl-C sends `INT`, a harness stopping a command sends `TERM` to its process group before anything harder, and each becomes an ordinary exit, which runs the `EXIT` trap. They are not redundant: bash runs an `EXIT` trap when a signal kills it, but zsh and dash do not. A temporary directory left beside a project makes the parent look worked on, and the next run finds a sibling it did not create. Only a `KILL`, which no shell can trap, still leaves one behind, and the failure table says what to do with it. **Nothing is initialized here.** The default recipe ends `git init -b main` because it has just deleted the only repository at that path. This one ends on the user's repository, their branch and their remote, which is the whole point of taking the long way round. @@ -174,12 +176,15 @@ git -C add -A -- . && git -C commit -m "Start from Pipelex/