Stop building generated code from unsanitized free text in skill instructions - #3442
ayushcodes10 wants to merge 20 commits into
Conversation
save result already had an answer file option for exactly this reason; adding equivalents for the question and correction fields makes every free text field on that command safe to pass through a file instead of a command line argument. graphify add gets a symmetric file input reading a JSON payload for url, author, and contributor. Neither command's existing positional/flag form changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Two of the generated skill references instructed the host agent to substitute raw, unsanitized values directly into generated Python or shell source. The add reference embedded a URL and optional author/contributor name inside a single quoted Python literal passed to python's inline script flag, so an embedded quote broke it and crafted input could run arbitrary Python. The query reference substituted the user's verbatim question and a full LLM generated answer, unbounded length, into a double quoted shell argument, where an embedded backtick or command substitution still executes. Both now write the value to a file with the agent's own file write tool first and pass only that file's path on the command line, never the content itself, so there is no injection surface left at all. Regenerated all fourteen platform skill variants and their expected fixtures from the updated shared fragments; every skillgen guard still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
![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 3 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds a shell-injection-safe path for graphify add and save-result by accepting --from-file (JSON payload of url/author/contributor/dir), --question-file, --answer-file, and --correction-file, each reading its value from a file whose only shell argument is the agent-controlled path. Requires exactly one of --question/--question-file and --answer/--answer-file, erroring otherwise, while the legacy positional/flag forms still work. Rewrites the agent skill references (across all generator variants and expected fixtures) to instruct writing free-text question/answer/correction and add-payloads to files instead of substituting them into command or heredoc strings.
Worth a look
- Missing --from-file operand is treated as the URL —
graphify/cli.py:1974· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- add --from-file crashes with KeyError instead of clean error when 'url' missing —
graphify/cli.py:1985· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Fixed /tmp save-result filenames race across concurrent sessions —
graphify/skills/claw/references/query.md:176· 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 — 691 functions depend on the 482 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 5 callers, 124 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
_make_graph()— 4 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_lessons_artifact_cannot_be_globbed_back_into_memory()— 0 callers, 6 callees
Verification — 691 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: 637 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
262 of 262 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/test_affected_cli.py— impact, full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— impact, 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_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.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_cluster.py— full-run-safetytests/test_codebuddy.py— impact, full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safetytests/test_csharp_enum_members.py— full-run-safetytests/test_csharp_field_generic_args.py— full-run-safetytests/test_csharp_generic_callsites.py— full-run-safety- … and 212 more
non-code file(s) changed (
graphify/skills/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 6 more finding(s) on lines outside this diff (see the check run).
A missing operand for the from file flag fell through to the positional url branch and treated the flag's own name as the url, a confusing blocked scheme error instead of pointing at the actual mistake. A payload missing the required url key raised a raw, unhandled KeyError instead of the same clean error message every other failure in this command already produces. Both now exit with a clear, single line error, matching malformed JSON and a missing file, which already went through the same path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
The prior fix wrote question/answer/payload content to a fixed, shared temp filename. Two concurrent graphify sessions, two agents or two terminal tabs on the same machine, following the same instructions at once would race on that one path: one session's write could overwrite or be read as another session's value. Each instruction now runs mktemp first to reserve a path unique to that run before writing anything, closing the race the same file based handoff was meant to avoid in the first place. Regenerated all fourteen platform skill variants and their expected fixtures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
|
Investigated this round's bot findings. Confirmed real, fixed (now at cea4992):
Regression tests added for both CLI fixes; all 14 platform skill variants and expected fixtures regenerated for the mktemp fix; all skillgen guards and the full test suite pass (same pre-existing, unrelated failures as before: a missing optional dependency and one flaky timing test, both confirmed unrelated to this change). |
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 2 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Replaces shell-inlined free text in save-result and add with file-based inputs to close a shell-injection surface (#3439): dispatch_command now accepts --question-file, --correction-file, and --from-file (a JSON payload with url plus optional author/contributor/dir) alongside the existing --answer-file, erroring when neither a value nor its file is supplied. Rewrites the skill references so agents mktemp unique paths, write question/answer/payload via a file-write tool, and pass only those paths to the CLI. Updates the ingest and reflect test suites to cover the new file-driven paths.
Worth a look
- --from-file crashes on non-object JSON payloads —
graphify/cli.py:1984· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Explain save command still inlines an untrusted node name —
graphify/skills/droid/references/query.md:340· 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 — 697 functions depend on the 488 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 8 callers, 124 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
_make_graph()— 4 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_lessons_artifact_cannot_be_globbed_back_into_memory()— 0 callers, 6 callees
Verification — 697 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: 643 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
262 of 262 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/test_affected_cli.py— impact, full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— impact, 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_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.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_cluster.py— full-run-safetytests/test_codebuddy.py— impact, full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safetytests/test_csharp_enum_members.py— full-run-safetytests/test_csharp_field_generic_args.py— full-run-safetytests/test_csharp_generic_callsites.py— full-run-safety- … and 212 more
non-code file(s) changed (
graphify/skills/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 6 more finding(s) on lines outside this diff (see the check run).
A payload that parsed as valid JSON but was not an object, a list, a bare string, a number, raised a raw TypeError from subscripting it with url, since json.JSONDecodeError alone only catches a syntax error, not a value of the wrong shape. Checks the parsed value is a dict before looking up url and names the actual type it got in the error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
The path and explain save commands still built their question argument from a template with a node label spliced in inline. A node label is not guaranteed free of shell characters either, since it can come from extracted document content, not just a source identifier. Both now write the question to a reserved unique file the same way the answer already does, closing the gap between two fields in the same command that were treated inconsistently. Regenerated the affected platform skill variants and their expected fixtures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
|
Investigated this round's bot findings. Confirmed real, fixed (now at d66f2b2):
Regression tests added for the non-object payload cases; affected platform skill variants and expected fixtures regenerated; all skillgen guards and the full test suite pass (same pre-existing, unrelated failures as before). |
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.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Replaces the shell-inlined graphify add and save-result invocations with file-backed inputs to eliminate command injection from untrusted free text: save-result now accepts --question-file/--correction-file alongside the existing --answer-file, requiring one of --question/--question-file, and add gains a --from-file mode that reads a JSON payload with a required url key plus optional author/contributor/dir. Rewrites the agent skill references to reserve unique mktemp paths, write values via a file-write tool, and pass only the paths to the CLI. Invalid or missing payloads (bad JSON, non-object, missing url, missing --from-file path) exit non-zero with an explanatory message.
Worth a look
- Untrusted graph node labels are still substituted directly into shell commands —
graphify/skills/copilot/references/query.md:279· 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 now rejects a present but empty --question argument —
graphify/cli.py:1480· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- add --from-file can traceback on invalid dir type —
graphify/cli.py:2000· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Windows add-watch skill requires POSIX mktemp —
graphify/skills/windows/references/add-watch.md:15· 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 — 699 functions depend on the 490 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 9 callers, 124 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
_make_graph()— 4 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_lessons_artifact_cannot_be_globbed_back_into_memory()— 0 callers, 6 callees
Verification — 699 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 23f2ffa (diverged from this PR's base — delta is approximate).
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: 645 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
262 of 262 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/test_affected_cli.py— impact, full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— impact, 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_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.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_cluster.py— full-run-safetytests/test_codebuddy.py— impact, full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safetytests/test_csharp_enum_members.py— full-run-safetytests/test_csharp_field_generic_args.py— full-run-safetytests/test_csharp_generic_callsites.py— full-run-safety- … and 212 more
non-code file(s) changed (
graphify/skills/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 6 more finding(s) on lines outside this diff (see the check run).
The node flag on save result still took node labels as literal command line words, even though a caller building this command from an agent identified label (the query, path, and explain references) cannot guarantee that label is free of shell characters, the same class of problem the question and answer file options already closed for that free text. The new option reads a newline separated list of labels from a file instead, mirroring the existing file backed options for the question and answer text; the direct node flag still works unchanged for callers that already control the label content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Save result checked its question and answer values by truthiness, so passing an explicit empty string for either was rejected with the same error as never passing that value at all, which is misleading since a value was in fact given. Check for None instead, which argparse only leaves in place when the value is truly absent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A payload whose dir key was a list, object, or number reached Path() unchecked and raised a raw TypeError instead of the same clean error message every other malformed payload shape in this command already produces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The query, path, and explain skill references already routed the free text question and answer through a file instead of substituting them into a command or inline script, but a cited node label still went into the node flag as a literal command line word and, for path and explain, straight into a Python string literal inside the inline fallback script. Both are the same injection surface the question and answer text already had closed for them: a node label can come from extracted document content, so an embedded quote, backtick, or a command substitution still corrupts or escapes whatever it lands in. Each flow now reserves one more temp path, writes the label or labels to it, and reads it back instead: the inline path and explain scripts read the term from that file, and every save result call now passes the file backed node option in place of the literal one. Regenerated all fourteen platform skill variants and their expected fixtures from the updated shared fragment; every skillgen guard still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers the same subprocess round trip as the existing question and answer file tests: adversarial content (backticks, a command substitution, both quote styles) written to a node file survives into the saved memory doc unexecuted and uncorrupted. Also covers the present but empty question fix from an earlier commit in this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed 5 new commits addressing the review findings:
Full test suite (5370 passed, 66 skipped, no regressions) and |
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.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Routes graphify save-result and graphify add through file-backed inputs, adding --question-file/--nodes-file/--correction-file (alongside the existing --answer-file) and an --add --from-file JSON payload so agent-built commands never inline unpredictable free text into a shell string. Validates the --from-file payload as a JSON object with a required url key and a string dir, erroring cleanly on bad paths, malformed JSON, or wrong types, while keeping the positional url + --author/--contributor/--dir form working. Rewrites the agents/query/add-watch skill references (and their generated per-tool copies) to write question/answer/node/payload values to mktemp-reserved files and pass only the paths to the CLI.
Worth a look
- --from-file forwards non-string URL to ingest —
graphify/cli.py:2002· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- add --from-file accepts non-string url —
graphify/cli.py:2003· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- --from-file 'url' value not validated as a string before reaching ingest() —
graphify/cli.py:2011· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- path/explain inline scripts read all node labels from single-line file but node file may contain multiple lines —
graphify/skills/amp/references/query.md:238· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- mktemp with .json suffix uses literal XXXXXX not a template —
graphify/skills/codex/references/add-watch.md· 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 — 705 functions depend on the 496 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 10 callers, 124 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
_make_graph()— 4 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_lessons_artifact_cannot_be_globbed_back_into_memory()— 0 callers, 6 callees
Verification — 705 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit c7ec108 (diverged from this PR's base — delta is approximate).
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: 651 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
262 of 262 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/test_affected_cli.py— impact, full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— impact, 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_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.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_cluster.py— full-run-safetytests/test_codebuddy.py— impact, full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safetytests/test_csharp_enum_members.py— full-run-safetytests/test_csharp_field_generic_args.py— full-run-safetytests/test_csharp_generic_callsites.py— full-run-safety- … and 212 more
non-code file(s) changed (
graphify/skills/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 6 more finding(s) on lines outside this diff (see the check run).
A JSON payload file whose "url" key was a list, object, or number
reached the ingest layer unchecked and surfaced a confusing
AttributeError ("'list' object has no attribute 'lower'") instead of
the same clean error message every other malformed payload shape in
this command already produces, mirroring the existing dir check.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BSD mktemp (the default on macOS) only substitutes a trailing run of X characters; a suffix written after them, like the .json extension this template had, comes back completely literal instead of random, so every agent session reserving a payload path got the exact same filename and the whole point of reserving a unique one was silently defeated. The add command's payload file option never looks at the extension, so dropping it is free. Every other reserved path in these references already puts the placeholder last; this was the one exception. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The two term unpack assumed the reserved file held exactly two non blank lines; a stray blank line from the file write tool (an extra trailing newline beyond the one strip already tolerated, or one left in the middle by mistake) would change the line count and crash the unpack. Filter blank lines first, matching how the node option on the CLI side already reads this same file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed 3 more commits addressing this round's findings:
Full suite (5373 passed, no regressions) and every |
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.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds a --from-file mode to graphify add and --question-file/--answer-file/--nodes-file/--correction-file inputs to save-result, so skill-generated commands read unpredictable free text (URLs, questions, answers, cited node labels) from files instead of interpolating it into shell command strings that quotes or $() could escape. Validates the add JSON payload — rejecting non-object bodies, a missing url, and non-string url/dir with distinct error messages — and reads nodes-file as one node per non-blank line. Rewrites the add-watch and query skill references to reserve unique mktemp paths, write values via a file-write tool, and pass only the paths to the new flags.
Worth a look
- from-file allows non-string author/contributor values into ingest —
graphify/cli.py:2014· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- from-file author/contributor are not type-validated —
graphify/cli.py:2021· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Path fallback cannot handle node names containing newlines —
graphify/skills/codex/references/query.md:235· 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 — 707 functions depend on the 498 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 11 callers, 124 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
_make_graph()— 4 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_lessons_artifact_cannot_be_globbed_back_into_memory()— 0 callers, 6 callees
Verification — 707 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit c7ec108 (diverged from this PR's base — delta is approximate).
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: 653 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
262 of 262 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/test_affected_cli.py— impact, full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— impact, 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_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.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_cluster.py— full-run-safetytests/test_codebuddy.py— impact, full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safetytests/test_csharp_enum_members.py— full-run-safetytests/test_csharp_field_generic_args.py— full-run-safetytests/test_csharp_generic_callsites.py— full-run-safety- … and 212 more
non-code file(s) changed (
graphify/skills/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 6 more finding(s) on lines outside this diff (see the check run).
Unlike a bad url or dir, a non string author or contributor does not crash today, since _yaml_str stringifies anything, but it silently saves a garbled frontmatter value like "['Jane']" instead of the clean error every other bad payload shape in this command already produces. Validated the same way for consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A file holding several node labels, one per line, cannot tell a label containing a literal newline apart from two separate labels. The node file option now takes one or more paths and reads lines from every one of them, so a caller with a fixed, known number of labels can reserve one file per label instead of relying on an assumed line count; a single path still works exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d file The two term unpack read a shared file assuming it held exactly two non blank lines, one per concept. A concept name containing a literal newline would split across lines and change that count, crashing the unpack instead of corrupting or silently misreading the names, but still a robustness gap worth closing outright: each concept name now gets its own reserved path, read as one whole value with no line splitting at all, so an embedded newline in the name itself cannot change how many values come back. The save result call now passes both paths to the node file option's new multi path form. Query and explain are unaffected, since query already cites a variable number of labels through one file on purpose and explain's single value was never split in the first place. Regenerated all fourteen platform skill variants and their expected fixtures from the updated shared fragment; every skillgen guard still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed 3 more commits:
Full suite (5380 passed, no regressions) and every `skillgen` guard green. |
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.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Replaces text-substituted invocations of graphify add and save-result with file-based inputs — --from-file reads a JSON {url, author, contributor, dir} payload, and --question-file/--answer-file/--correction-file/--nodes-file read their values from files — so skills building commands from unpredictable user/LLM text never inline quotes or shell metacharacters into a command string (#3439). Keeps positional-URL and --author/--contributor/--nodes flag forms working, with validation errors for malformed payloads and a missing --from-file path. Updates the skill references across all agent variants to mktemp a unique payload path, write JSON with a file tool rather than a heredoc, and pass only that path to the CLI.
Worth a look
- save-result no longer rejects empty answers —
graphify/cli.py:1494· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Path save-result example passes two files to a singular --nodes-file option —
graphify/skills/claw/references/query.md:300· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Path-query docs pass two operands to a singular --nodes-file option —
graphify/skills/pi/references/query.md:298· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Documented path save-result command passes two arguments to a singular --nodes-file option —
graphify/skills/trae/references/query.md:297· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Path save-result documents ambiguous multi-file --nodes-file usage —
graphify/skills/vscode/references/query.md:293· 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 — 711 functions depend on the 502 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 12 callers, 124 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
_make_graph()— 4 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_lessons_artifact_cannot_be_globbed_back_into_memory()— 0 callers, 6 callees
Verification — 711 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit c7ec108 (diverged from this PR's base — delta is approximate).
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: 657 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
262 of 262 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/test_affected_cli.py— impact, full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— impact, 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_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.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_cluster.py— full-run-safetytests/test_codebuddy.py— impact, full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safetytests/test_csharp_enum_members.py— full-run-safetytests/test_csharp_field_generic_args.py— full-run-safetytests/test_csharp_generic_callsites.py— full-run-safety- … and 212 more
non-code file(s) changed (
graphify/skills/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 6 more finding(s) on lines outside this diff (see the check run).
Checking only whether the value was None let an explicitly empty question or answer through as if it were meaningful content to save, which it never is. Both are still rejected, but the message now says the value must not be empty instead of the misleading required message a genuinely absent flag gets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The path fallback's example passes two paths to it, which read as a possible mistake next to query and explain's own single path examples elsewhere in the same document. A short note next to the first usage says outright that both forms are valid. Regenerated all fourteen platform skill variants and their expected fixtures from the updated shared fragment; every skillgen guard still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed 2 more commits:
Full suite (5381 passed, no regressions) and every |
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 2 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Replaces free-text substitution in the graphify add and save-result command paths with file-based input to eliminate shell-injection risk when skill instructions build commands from user URLs, names, questions, and generated answers (#3439). Adds --from-file to add (reading a validated JSON {url, author?, contributor?, dir?} payload) and --question-file/--answer-file/--nodes-file/--correction-file to save-result, with empty question/answer now rejected distinctly and --nodes-file splitting labels per file rather than assuming a fixed line count. Rewrites the add-watch and query skill references to write a mktemp-reserved payload file with the agent's file-write tool and pass only its path, dropping the inline python -c snippets.
Worth a look
- save-result --answer-file now rejects empty files that were previously accepted —
graphify/cli.py:1485· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- add --from-file crashes on invalid UTF-8 payload —
graphify/cli.py:2002· 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 — 712 functions depend on the 503 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 12 callers, 124 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
_make_graph()— 4 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_lessons_artifact_cannot_be_globbed_back_into_memory()— 0 callers, 6 callees
Verification — 712 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit eaaec1a (diverged from this PR's base — delta is approximate).
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: 658 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
262 of 262 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/test_affected_cli.py— impact, full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— impact, 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_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.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_cluster.py— full-run-safetytests/test_codebuddy.py— impact, full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safetytests/test_csharp_enum_members.py— full-run-safetytests/test_csharp_field_generic_args.py— full-run-safetytests/test_csharp_generic_callsites.py— full-run-safety- … and 212 more
non-code file(s) changed (
graphify/skills/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 6 more finding(s) on lines outside this diff (see the check run).
A payload file that was not valid UTF 8, binary content or a stray byte, raised a raw, unhandled UnicodeDecodeError from read_text instead of the same clean error message every other unreadable payload shape in this command already produces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Re: the two new findings. "add --from-file crashes on invalid UTF-8 payload": real, confirmed by reproduction — a payload file with invalid UTF-8 bytes raised a raw, unhandled `UnicodeDecodeError` from `read_text()` instead of the clean error every other unreadable-payload shape already gets. Pushed a fix: the read is now also guarded against `UnicodeDecodeError`, matching the existing `OSError`/`JSONDecodeError` handling right next to it. "save-result --answer-file now rejects empty files that were previously accepted": accurate description, but intentional — this is the same fix from an earlier round in this thread (an explicitly empty `--answer`/`--question` is never meaningful content to save, so it's rejected with a clear "must not be empty" message rather than silently saved). That check applies uniformly regardless of whether the empty value came from the flag directly or from an empty file; a blank `--answer-file` was accepted before only because that path had no post-read validation at all, which was the gap the earlier fix closed. Not reverting this — flagging it as confirmed-intentional rather than silently letting it sit unaddressed. Full suite (5382 passed, no regressions) and `skillgen --check` green. |
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 2 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Replaces inline Python/shell command construction in graphify add and save-result skill flows with file-based payload passing to eliminate shell-injection risk from unpredictable user text (#3439). save-result now accepts --question-file, --correction-file, and multi-path --nodes-file (one file per node label, splitting per file to avoid a fixed-line-count unpack), and rejects empty question/answer values; add gains --from-file taking a JSON payload with a required string url and optional author/contributor/dir, validating each type and erroring on malformed input. Updates the agent skill references to write payloads via mktemp-reserved unique paths and pass only the path, not the content, to the CLI.
Worth a look
- file reads for --question-file/--answer-file/--correction-file/--nodes-file have no error handling —
graphify/cli.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Untrusted add payload is documented as raw JSON interpolation —
graphify/skills/amp/references/add-watch.md:26· 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 — 714 functions depend on the 505 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 13 callers, 124 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_run_hook_guard()— 4 callers, 8 callees - new:
_make_graph()— 4 callers, 6 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees - new:
test_lessons_artifact_cannot_be_globbed_back_into_memory()— 0 callers, 6 callees
Verification — 714 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit eaaec1a (diverged from this PR's base — delta is approximate).
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: 660 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
262 of 262 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/test_affected_cli.py— impact, full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— impact, 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_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.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_cluster.py— full-run-safetytests/test_codebuddy.py— impact, full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safetytests/test_csharp_enum_members.py— full-run-safetytests/test_csharp_field_generic_args.py— full-run-safetytests/test_csharp_generic_callsites.py— full-run-safety- … and 212 more
non-code file(s) changed (
graphify/skills/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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/agents/references/add-watch.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/add-watch.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/add-watch.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
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 6 more finding(s) on lines outside this diff (see the check run).
1 participant
Fixes #3439.
Two of the generated skill reference docs instructed the host agent to substitute raw, unsanitized values directly into generated Python or shell source.
add-watch.md(the/graphify addflow)The instructions had the agent embed a URL and optional author/contributor name inside a single-quoted Python string literal, passed to
python -c "...":An embedded apostrophe breaks the Python syntax; crafted input runs arbitrary Python.
query/default.md(all threesave-resultcall sites: query, path, explain)The instructions had the agent substitute the user's verbatim question and a full LLM-generated answer (unbounded length, arbitrary content) into a double-quoted shell argument:
Double quotes still allow command substitution/backticks, and embedded quotes break argument boundaries.
Fix
save-resultalready had--answer-filefor exactly this reason (#1502). This PR:--question-file/--correction-fileforsave-result, and a--from-file(JSON payload:url/author/contributor/dir) forgraphify add. All existing positional/flag forms are unchanged — purely additive.tools/skillgen/expected/fixtures from the updated shared fragments.Test plan
pytest tests/test_ingest.py tests/test_reflect.py— new tests cover--question-file/--correction-fileround-tripping adversarial content (backticks,$(), both quote styles) throughsave-result, andgraphify add --from-filepassing an adversarial JSON payload through toingest()unexecuted (mocked, network-free) plus a check that the original positional-arg form still works unchangedpython -m tools.skillgen --checkand all guard modes (--audit-coverage,--schema-singleton,--monolith-roundtrip,--always-on-roundtrip) passtest_ollama_retry_cap.py, missingopenaimodule in this environment;test_ts_normalizer_scales_linearly_on_large_files, a flaky wall-clock timing assertion, confirmed to pass in isolation)🤖 Generated with Claude Code
https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh