Repository navigation
Conversation
|
Thanks for the pull request, @wannT-1. 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. |
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.
Not checked: tests were not run; formal verification proved 0 of 5 changed function(s) (3 sampled, not proven, 2 not verified).
Formal verification. PR-changed functions: 3/5 verified (0 proven, 3 may-equivalent, 0 distinguished) · 2 not verified (2 vacuous).
Not verified on this run: \_translate\_bash\_block (vacuous: never exercised), render (vacuous: never exercised).
Graphify review — findings
Adds PowerShell-native versions of the Windows skill's reference docs, replacing the bash blocks with powershell fences generated by the skillgen translator. Inline Python now runs through a single-quoted here-string piped into the interpreter path read from .graphify_python, line continuations become backticks, and ~ becomes $HOME; the tests assert no bash leftovers, no BOM on written marker files, and that POSIX hosts keep their bash invocations. The piped-here-string and double-quoted interpolation forms are where this can go wrong: query, path and explain arguments are expandable strings, non-ASCII text can be mangled on its way to Python, and --watch fails when .graphify_root is empty.
Worth a look
- PowerShell query/path/explain examples use injectable expandable strings —
graphify/skills/windows/references/query.md:64· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
- PowerShell transcription env var examples allow quote-breakout injection —
graphify/skills/windows/references/transcribe.md:26· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
- Piped here-string mangles non-ASCII in Windows PowerShell 5.1 —
graphify/skills/windows/references/query.md:19· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
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 — 405 functions depend on the 383 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
render()— 17 callers, 7 callees - worse:
_translate_bash_block()— 3 callers, 12 callees
Verification — 405 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: 405 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
518 of 518 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_4874_jvm_resolution.py— full-run-safetytests/test_4909_reference_binding_scale.py— 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_affected_path_file_node.py— full-run-safetytests/test_affected_seed_reference_forms.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_backend_opt_in.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_binary_media_words.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_ghost_fold_4955.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_long_paths.py— full-run-safetytests/test_cache_stale_import_target.py— full-run-safetytests/test_cache_windows_case_4925.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_only_decl_ids_4904.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-safety- … and 468 more
non-code file(s) changed (
graphify/skills/windows/references/add-watch.md,graphify/skills/windows/references/exports.md,graphify/skills/windows/references/github-and-merge.md,graphify/skills/windows/references/hooks.md,graphify/skills/windows/references/query.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/skills/windows/references/add-watch.md,graphify/skills/windows/references/exports.md,graphify/skills/windows/references/github-and-merge.md,graphify/skills/windows/references/hooks.md,graphify/skills/windows/references/query.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 \_core\_to\_powershell (not a proof).
The verifier ran both versions of \_core\_to\_powershell 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.
Could not verify: Could not verify \_translate\_bash\_block.
The verifier did not have enough to check \_translate\_bash\_block, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: non-vacuity: domain too small (only 1 distinct inputs exercised, need 3) — 'no divergence' would be near-vacuous
No difference found (not proven): No behavior difference found in \_translate\_prose\_line (not a proof).
The verifier ran both versions of \_translate\_prose\_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.
No difference found (not proven): No behavior difference found in \_unescape\_bash\_dq (not a proof).
The verifier ran both versions of \_unescape\_bash\_dq 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.
Could not verify: Could not verify render.
The verifier did not have enough to check render, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 3 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly ValueError — names the real obstacle, not a sampling gap)
· 1 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).
| return line[: len(line) - len(line.lstrip())] | ||
|
|
||
|
|
||
| def _translate_bash_block(lines: list[str]) -> list[str]: |
There was a problem hiding this comment.
_translate_bash_block()
fans out to 12 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
The strict POSIX-to-PowerShell pass only ran on the core skill body, so windows references still shipped bash fences. Run that translator over references when shell is powershell, including a mktemp path-capture rule, and regenerate the Windows artifacts. Fixes Graphify-Labs#3600 Co-authored-by: wannT-1 <wannT-1@users.noreply.github.com>
bb67e93 to
bae295a
Compare
What does this PR do?
Fixes #3600.
Windows skill references were copied from the shared POSIX fragments even when
shell = "powershell". The strict translator already rewrote the core skill body, sographify/skills/windows/references/*.mdstill shipped bash fences:$(cat ...),export,if [ ! -f ],cp/rm -f, and (where a fragment uses it)mktemp /tmp/graphify_*.XXXXXX.This runs
_core_to_powershelloverreferences/*.mdfor powershell-shell hosts and extends that translator with the reference patterns, still failing the render on any bash line it does not recognize.mktempcapture:VAR=$(mktemp /tmp/graphify_*.XXXXXX)becomes$VAR = [System.IO.Path]::GetTempFileName()mktemp /tmp/graphify_*.XXXXXX(the form that prints the path) becomes the same call with no assignmentGetTempFileName()creates an empty temp file and returns its full path, so the path is captured the way$(mktemp ...)was. The/tmp/graphify_*template is not passed through; any othermktempshape still fails the render.Other reference POSIX that the shared fragments actually contain is translated the same way (
$(cat .graphify_python) -m,"$(cat .graphify_root)",export,if [ ! -f ],VAR=$(graphify ...), bash backslash continuations, and~/on those commands). POSIX hosts are unchanged. The core Windows skill body is unchanged.Type of change
Verification & Invariants
The powershell render stays strict: an unrecognized bash line in a Windows reference raises at render time, and banned bash tokens cannot survive into the artifact.
How was this tested?
76 skillgen tests passed.
tools.skillgen --checkmatched all 134 artifacts. Ruff reported no issues.Graphify-specific checklist