fix(skillgen): quote .graphify_python in reference fragments for paths with spaces - #2905
Conversation
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Quote $(cat graphify-out/.graphify_python) in every skill reference doc across all agent variants (agents, amp, claude, codex, copilot, droid, kilo, kiro, opencode, pi, trae, vscode, windows) and their skillgen expected fixtures. Fixes command failures when the resolved Python interpreter path contains spaces.
Worth a look
- Verbatim question interpolation in shell command enables injection —
graphify/skills/vscode/references/query.md:171· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- User-controlled save-result arguments are interpolated into a shell command —
graphify/skills/windows/references/query.md:171· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Verbatim user question is interpolated into a shell command inside double quotes —
graphify/skills/claw/references/query.md:171· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Verbatim question is interpolated into a shell command inside double quotes —
graphify/skills/copilot/references/query.md:168· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Quoting .graphify_python rejects multi-word interpreter commands —
graphify/skills/kilo/references/query.md:14· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 754 functions depend on the 754 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 754 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 754 function(s) in the blast radius were not formally verified this run
…s with spaces Unquoted $(cat graphify-out/.graphify_python) in shared reference fragments word-splits on Windows when the cached interpreter path contains a space. Quote the substitution in the five reference source fragments and regenerate skill artifacts plus expected/ baselines. Fixes Graphify-Labs#2856
The Graphify reviewer flagged five advisory findings on PR Graphify-Labs#2905: the save-result template at the end of every skill's query.md interpolates the user's verbatim question and answer into a shell command inside double quotes. A question like `hello"; rm -rf /` could escape the argument and break out of the command. Switch the user-supplied values to single quotes (literal in bash, no expansion) and document the standard `'\\''` escape pattern for embedded single quotes. Keep the double-quoted `"\$(cat ...)"` around .graphify_python: the file is documented to hold a single executable path, not a multi-word command, so the multi-word-command concern from the kilo finding is a false positive. Touched via the central source fragment `tools/skillgen/fragments/references/query/default.md`; all 14 split platform references and the expected/ fixtures were regenerated with `python3 -m tools.skillgen --bless`. - tools/skillgen --check: 134 artifacts OK - pytest tests/test_skillgen.py: 64 passed
21018d0 to
5a34411
Compare
|
Addressing the 5 advisory findings from graphify-labs on 2026-08-20:
Touched via the central source fragment
|
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Graphify review — findings
Quotes the $(cat graphify-out/.graphify_python) command substitution across all add-watch, exports, query, transcribe, and update reference docs (and their skillgen expected fixtures) for every agent variant. Switches the save-result query/path/explain examples to single-quoted user-supplied values and documents the '\'' escape, making the token substitution injection-safe. Updates the corresponding tools/skillgen expected outputs to match.
Worth a look
- URL placeholder is embedded in shell-evaluated Python source —
graphify/skills/kilo/references/add-watch.md:14· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- save-result substitution remains injection-prone despite single-quote guidance —
graphify/skills/amp/references/query.md:174· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Command injection via unquoted NODE_A/NODE_B/NODE_NAME/QUESTION placeholders still remaining —
graphify/skills/opencode/references/query.md:168· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 754 functions depend on the 754 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 754 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 754 function(s) in the blast radius were not formally verified this run
Follow-up to Graphify-Labs#2905 addressing three new Graphify advisory findings on the post-fix render (review at 5a34411): - HIGH (kilo/references/add-watch.md:14) — URL placeholder embedded in shell-evaluated Python source. The 'URL' literal sits inside the python -c '...' block, so user-supplied text replaces a Python literal rather than flowing through a string parameter. Switching to sys.argv reads keeps user text out of evaluated Python source. - MEDIUM (amp/references/query.md:174, opencode/references/query.md:168) — --nodes NODE1 NODE2 left node labels unquoted after the question/answer single-quote fix. Node labels from the graph can contain whitespace, single quotes, semicolons, or shell metacharacters; substituting them unquoted preserves the original command-injection surface. Each node label is now single-quoted with the standard '\'' escape documented alongside the question/answer pattern. Same fix applies to all three save-result blocks in the source fragment (query, path_query, explain) since the unquoted --nodes pattern repeats across all three. Source-of-truth fragments edited, then all 14 platform renders + matching expected/ fixtures regenerated via 'python -m tools.skillgen --bless'. --check / --audit-coverage / --schema-singleton / --monolith-roundtrip / --always-on-roundtrip all green; tests/test_skillgen.py 64/64 pass.
|
Addressing the 3 new Graphify advisory findings from the re-review at 5a34411:
Source-of-truth fragments edited ( Verification:
Head now |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 4 advisory finding(s) below merit a look before merge.
Graphify review — findings
Quote $(cat graphify-out/.graphify_python) throughout the graphify skill reference docs so the Python path survives spaces, and rework add-watch.md, query.md, and the save-result snippets to pass user-supplied values (URLs, questions, answers, node labels) as separate sys.argv/single-quoted shell arguments instead of interpolating them into evaluated source. Adds injection-safety guidance covering '\'' escaping for literal single quotes, mirrored across all agent variants (claude, codex, opencode, pi, kiro, trae, droid, claw, windows, vscode, copilot, etc.) and their tools/skillgen expected/fragment fixtures.
Worth a look
- Whisper prompt export evaluates command substitutions —
graphify/skills/agents/references/transcribe.md:28· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Single-quoted save-result substitution remains command-injectable for questions/answers containing single quotes —
graphify/skills/claw/references/query.md:171· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Query save-result docs omit escaping apostrophes in question/answer —
tools/skillgen/expected/graphify__skills__amp__references__query.md:174· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Query save-result quoting guidance leaves question/answer apostrophes unsafe —
tools/skillgen/expected/graphify__skills__copilot__references__query.md:171· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 638 functions depend on the 638 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 638 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 638 function(s) in the blast radius were not formally verified this run
…r export to single quotes Follow-up to Graphify-Labs#2905 addressing four new Graphify advisory findings on the post-fix render (review at 12c3b67): - HIGH (graphify/skills/agents/references/transcribe.md:28) — the `export GRAPHIFY_WHISPER_PROMPT="<...>"` pattern left the one-sentence domain hint inside double quotes, so a hint containing ``, `$(...)`, or `"`, or an LLM-generated hint with shell metacharacters, would be evaluated by bash during export. Switching to single-quoted assignment plus a separate `export` line keeps the prompt literal; the standard `'\\''` escape pattern is documented in-line for hints that themselves contain an apostrophe. - MEDIUM (graphify/skills/claw/references/query.md:171, tools/skillgen/expected/graphify__skills__amp__references__query.md:174, tools/skillgen/expected/graphify__skills__copilot__references__query.md:171) — the prose around the save-result --question/--answer/--nodes block only documented the `'\\''` escape for *node labels*, leaving ORIGINAL_QUESTION and ANSWER without explicit guidance. A question like `What's the difference?` substituted into `'ORIGINAL_QUESTION'` produced a bash syntax error (unmatched single quote). Sabotage run confirms the old pattern fails with `unexpected EOF while looking for matching '\''`; the new pattern with `'\\''` escape parses cleanly. The escape guidance now explicitly names ORIGINAL_QUESTION, ANSWER, and node labels as the three substituted values that need it. Source-of-truth fragments edited (`tools/skillgen/fragments/references/shared/transcribe.md`, `tools/skillgen/fragments/references/query/default.md`), then all 14 platform renders + matching `expected/` fixtures regenerated. Verification: - `python -m tools.skillgen --check` → OK, 134 artifacts match - `python -m tools.skillgen --audit-coverage` → OK - `python -m tools.skillgen --schema-singleton` → OK - `python -m tools.skillgen --monolith-roundtrip` → OK - `python -m tools.skillgen --always-on-roundtrip` → OK - `python -m pytest tests/test_skillgen.py` → 64/64 pass - Sabotage run: old save-result pattern with apostrophe-containing question → bash syntax error; new pattern → parses cleanly. Old double-quoted Whisper export with `$(touch ...)` injection → executes the command (file created); new single-quoted export → keeps the value literal (no file created).
|
Addressing the 4 new Graphify advisory findings from the re-review at 12c3b67:
Source-of-truth fragments edited ( Verification:
Head now |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Quote all $(cat graphify-out/.graphify_python) command substitutions and switch save-result/ingest/whisper-prompt invocations to pass user-supplied values as single-quoted sys.argv/shell arguments instead of interpolating them into Python source, closing a shell/Python injection vector in the graphify skill reference docs. Adds guidance on escaping literal single quotes ('\'') across all agent variants (agents, codex, claude, copilot, opencode, droid, etc.) and their skillgen expected-output fixtures.
Worth a look
- Quoted .graphify_python invocation no longer supports interpreter commands with arguments —
graphify/skills/trae/references/query.md:14· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 638 functions depend on the 638 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 638 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 638 function(s) in the blast radius were not formally verified this run
Acknowledging the 4th re-review (97bc499) — pausing iteration pending maintainer inputThe new bot review at
The bot itself marks this finding "NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review". Position on the trae finding
If So I'm holding the Status
Happy to walk through any of the four commits on a maintainer review call. Will not push new changes until then. |
|
Re the residual medium advisory on
Re-verified on the current head (97bc499):
No further code change is needed for this advisory; it's a documented trade-off for the |
1 participant
Problem
tools/skillgen/fragments/references/render bash snippets with unquoted command substitution$(cat graphify-out/.graphify_python). Bash word-splits the path, so every query/update/add-watch/exports/transcribe step fails on Windows when the cached interpreter path contains a space (e.g.C:\Users\Jane Doe\...).graphify/skill-windows.mdwas already migrated to the space-safe PowerShell reader& (Get-Content graphify-out\.graphify_python), but the shared reference fragments were not.Fixes #2856
Fix
Quote the substitution as
"$(cat graphify-out/.graphify_python)"in the five reference source fragments (18 call sites):references/query/default.md(8)references/shared/update.md(6)references/shared/add-watch.md(2)references/shared/transcribe.md(1)references/shared/exports.md(1)Regenerated all platform
graphify/skills/*/references/artifacts and updatedtools/skillgen/expected/baselines viapython3 -m tools.skillgen+--bless.Test
python3 -m tools.skillgen --check— OK (134 artifacts match committed output and expected/)python3 -m pytest tests/test_skillgen.py -q— 63 passed, 1 failed (test_no_version_or_timestamp_in_outputfalse-positive on the literal"unknown"default in query.md; pre-existing on v8, unrelated to this diff)