Repository navigation
fix(skill): drop backticks bash ran inside Part C and the update merge (#4461) - #4462
Deepjyoti-Sarmah wants to merge 2 commits into
Conversation
Checks every generated skill file statically, and runs the Part C and update merge statements under bash with a stub interpreter. This commit alone fails 58 cases (29 per check); the next commit fixes them.
Graphify-Labs#4461) Bash expands backticks inside a double-quoted -c block, so two comments ran graphify update and a command named changed before Python started. Drop the backticks and keep the words; regenerate with --bless. The gen.py monolith audit accepts the new Part C comment text.
|
Thanks for the pull request, @Deepjyoti-Sarmah. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
github-actions
Bot
added
the
contributor
![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.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; formal verification proved 0 of 1 changed function(s) (1 sampled, not proven).
Formal verification. PR-changed functions: 1/1 verified (0 proven, 1 may-equivalent, 0 distinguished) · 0 not verified.
Graphify review — findings
Removes the backticks around graphify update in the _origin comment of the semantic-merge snippet in every platform skill (agents, aider, amp, claw, codex, copilot, devin, droid, kilo, kiro, opencode, pi, trae, vscode, windows and the default), so the shell no longer treats them as command substitution inside the double-quoted Python block. Updates the skillgen fragments and expected outputs so regenerated skills keep the plain-text wording. Adds a static test using backtick_lines_in_dq_blocks to flag backticks inside double-quoted shell blocks, though it does not yet cover single-line -c "..." strings.
Review partial — this diff was larger than one review pass covers, so later files were not reviewed; some findings may be missing.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1258 functions depend on the 1178 functions this change touches.
Health — grade A; 5 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
render()— 14 callers, 5 callees (high)audit_coverage()— 8 callers, 6 callees (high)main()— 3 callers, 11 callees (medium)monolith_roundtrip()— 3 callers, 5 callees (medium)test_audit_catches_a_dropped_non_allowlisted_heading()— 0 callers, 6 callees (medium)
Verification — 1258 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: 1258 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
419 of 419 test file(s) selected (100%) via static blast radius.
Escalated to a full run for safety — the selection is not trustworthy on its own (see below). CI should run the whole suite.
tests/pi.test.ts— full-run-safetytests/test_affected_cli.py— full-run-safetytests/test_affected_defines.py— full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_affected_overrides.py— full-run-safetytests/test_agents_platform.py— full-run-safetytests/test_alias_folding.py— full-run-safetytests/test_analyze.py— full-run-safetytests/test_anthropic_custom_endpoint.py— full-run-safetytests/test_antigravity_install.py— full-run-safetytests/test_apm_fallback_version.py— full-run-safetytests/test_architecture_doc.py— full-run-safetytests/test_astro_extraction.py— full-run-safetytests/test_astro_import_ids.py— full-run-safetytests/test_atomic_canvas_export.py— full-run-safetytests/test_atomic_version_stamp.py— full-run-safetytests/test_atomic_writes.py— full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_benchmark_real_query.py— full-run-safetytests/test_blade_extractor.py— full-run-safetytests/test_build.py— full-run-safetytests/test_build_located_semantic_identity.py— full-run-safetytests/test_build_merge_dedup_scope.py— full-run-safetytests/test_build_merge_hyperedges_and_prune.py— full-run-safetytests/test_build_merge_shrink_guard.py— full-run-safetytests/test_builtin_global_type_refs.py— full-run-safetytests/test_c_include_resolution.py— full-run-safetytests/test_cache.py— full-run-safetytests/test_cache_stale_import_target.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— full-run-safetytests/test_cargo_missing_manifest.py— full-run-safetytests/test_carried_hyperedge_remap.py— full-run-safetytests/test_case_sensitive_resolution.py— full-run-safetytests/test_charmap_encoding.py— full-run-safetytests/test_chunking.py— full-run-safetytests/test_cjs_module_extension.py— full-run-safetytests/test_claude_cli_backend.py— full-run-safetytests/test_claude_md.py— full-run-safetytests/test_cli_broken_pipe.py— full-run-safetytests/test_cli_export.py— full-run-safetytests/test_cli_help.py— full-run-safetytests/test_cloud_cta.py— full-run-safetytests/test_cluster.py— full-run-safetytests/test_cluster_ambiguous_scale.py— full-run-safetytests/test_cluster_exclude_hubs.py— full-run-safetytests/test_cobol_extractor.py— full-run-safety- … and 369 more
non-code file(s) changed (
graphify/skill-agents.md,graphify/skill-aider.md,graphify/skill-amp.md,graphify/skill-claw.md,graphify/skill-codex.md…) → running the full suite for safety (a code graph can't see config/fixture/data deps)
changed code file(s) with no mapped test (
graphify/skill-agents.md,graphify/skill-aider.md,graphify/skill-amp.md,graphify/skill-claw.md,graphify/skill-codex.md…) — a coverage gap or a missing link — running the full suite rather than only the selected tests
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
No difference found (not proven): No behavior difference found in \_is\_semantic\_origin\_stamp\_fix\_line (not a proof).
The verifier ran both versions of \_is\_semantic\_origin\_stamp\_fix\_line on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
|
Landed in 0.9.92 (authorship-preserving cherry-pick, so it shows on your contributions). Thanks @Deepjyoti-Sarmah! Released to PyPI: https://pypi.org/project/graphifyy/0.9.92/ · https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.92 |
2 participants
What does this PR do?
Fixes #4461.
Four comment lines inside double-quoted
-c "..."blocks had backticks. Bash expands backticks inside double quotes, so it ran the text before Python started. This PR drops the backticks, keeps the words, and adds a test that fails on any backtick inside such a block.Summary
The skills run Python as
$(cat graphify-out/.graphify_python) -c "...". Withgraphifyon PATH, bash ran a fullgraphify updatein the middle of Part C and pasted its output into the Python source. The first output line landed in the comment. The next lines were pasted into the code, and Part C failed withSyntaxError..graphify_extract.jsonwas never written. Withoutgraphifyon PATH the only sign wascommand not foundon stderr, so nobody noticed.Two commits:
test(skill): no command runs from backticks in -c "..." blocks:tests/test_skill_no_backtick_substitution.py. On its own it fails 58 cases (29 per check).fix(skill): drop backticks bash ran inside Part C and the update merge (#4461): the 4 fragment lines, the regenerated files, and onegen.pyline.Evidence
Test before and after: unfixed v8 58 failed, 105 passed. After the fix 163 passed. The 58 are 29 files per check: 15 monoliths plus 14
references/update.md.Real graphify on PATH (Part C copied verbatim from
graphify/skill.md:377-417, a project with a previous build and one edited file, venvgraphifyfirst on PATH):Other cases, v8: a fake
graphifyprinting 3 lines gives exit 1 andSyntaxError; one printing 1 line works, because the line lands inside the comment; nographifyon PATH gives exit 0 withcommand not found. A fakechangedprinting 3 lines in the--updatemerge block gives exit 1 andSyntaxError, withgraph.jsonandmanifest.jsonuntouched.The
# Save scan root so \graphify update` ...lines in Step 1,shell/posix.mdandshell/powershell.mdare plain shell comments outside quotes. Checked: a fakegraphify` was not called.Merge Danger
Door: two-way. Comment text only, in four fragment lines. No stored format changes, and reverting is one commit.
Blast Radius: narrow. Comments inside Python source the skills hand to the interpreter. Nothing in
graphify/*.pychanges.Cache and ids: no change. The semantic cache key comes from
prompt_fingerprint, which hashes the file the skill passes asprompt_file,references/extraction-spec.md(cache.py). That file is not in this diff. aider and devin pass no prompt. No node ids, graph format or manifest change.Generated files: 60, all regenerated with
--blessand never hand-edited: 16 monoliths and 14references/update.md, plus their 30tools/skillgen/expected/copies.skill-windows.mdis one of the 16. Its text changes, but it was never affected, because its Part C sits in a single-quoted PowerShell here-string (@').gen.py: one line._is_semantic_origin_stamp_fix_linematched the Part C comment with its backticks, so the round-trip audit would reject the new text. It now matches the text without backticks. The old string is not needed: the pinned v8 baselines for aider and devin hold neither form, and--monolith-roundtrippasses.Open PRs, checked with
git merge-tree --write-treeagainst each branch's head, then the new test run on the merged tree:"$(cat graphify-out/.graphify_python)" -c ". The test accepts both spellings, so it runs on their merged trees as well.-cblock (core.md:208). Its branch now drops them too (75218d9), so the new test passes whichever lands first.tests/test_code_only_update_all_hosts.py:156-160, which exists because of this bug. Not changed here.Not covered
$(cat graphify-out/.graphify_python) -c "..."statements in fenced bash blocks (quoted or not). Other shapes, such as"$PYTHON" -c "...", are not run; the static check still scans them.$VARexpansion inside the double quotes is not a command, so neither check looks at it.$(...)inside the quotes; the runtime check does, through the shell trace, for the statements it runs.@'...'@here-strings on purpose. The runtime test skips on win32.Type of change
Verification & Invariants
Invariant: no command runs while bash expands a skill's double-quoted
-c "..."block, so the Python source Part C and the update merge hand to the interpreter is exactly what the skill text says.tests/test_skill_no_backtick_substitution.pychecks it two ways overgraphify/skill*.mdandgraphify/skills/**(128 files). Static: no backtick inside any double-quoted-cblock, with PowerShell@'...'@here-strings skipped. Runtime: each-c "..."statement of the 29 files that hold Part C or the--updatemerge is run under bash with a stub interpreter, with a fakegraphifyand a fakechangedfirst on PATH. The shell is traced, andPATHholds only the fakes,catand the stub, so a backtick naming any other command is a trace line the test rejects.It has scanner unit tests and two runtime controls, so it cannot pass with nothing in it: a backticked
graphify updateis caught through the fake, and a backticked unknown command is caught through the trace. A guard test fails if the Part C and update files are not found.Generated or persisted state this invalidates: none.
--blessrewrites 30expected/files (the other 104 are content-identical).Read the CONTRIBUTING.md guide.
Reproduced the issue and identified the invariant.
Made the smallest fix necessary.
Added a regression test (if bug fix) or isolated boundary test.
Kept the PR description synchronized with the final implementation.
Documented any limitations / unsupported cases explicitly.
How was this tested?
Run from
/var/tmpwithPYTHONPATHset to the worktree and the repo venv's Python, so the code under test is this branch.graphify.__file__was checked to be inside the worktree.Not run:
pyrighton the whole repo.Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments.