Repository navigation
fix(skill): simplify credential and dispatch guidance - #4110
venkatpachala wants to merge 2 commits into
Conversation
|
Thanks for the pull request, @venkatpachala. 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 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Relaxes the generated graphify skills' "do not skip steps" rule so agents skip only where explicitly told: the existing-graph fast path, Step 0 for a local path, Step 2.5 without video/audio, and Part B for code-only or fully cached corpora. Replaces the "MANDATORY: use the Agent tool" mandate with platform-neutral dispatch guidance. Agents now skip dispatch when the fast path applies, Gemini handles semantic extraction, or everything is cached, and extract inline on hosts without subagents. The API-key note now routes code-only runs through Part B's fast path to write the empty semantic file, and points to graphify.llm.detect_backend() as the headless CLI's way to reach other providers.
Worth a look
- Top-level instructions say to skip Part B for code-only/fully-cached corpora despite Part B being required to materialize semantic output —
graphify/skill-kilo.md:60· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Top-level instruction incorrectly says to skip Part B when its output file is still required —
graphify/skill-copilot.md:59· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Top-level instruction says to skip Part B for code-only corpora, bypassing required semantic stub creation —
graphify/skill-agents.md:60· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Top-level instruction says to skip Part B for code-only corpora, bypassing required semantic stub creation —
graphify/skill-amp.md:60· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Code-only flow is told to skip Part B despite requiring its fast path output —
graphify/skill-claw.md:60· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; 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 — 870 functions depend on the 870 functions this change touches.
Health — this change adds coupling hotspots:
- new:
test_audit_catches_a_dropped_non_allowlisted_heading()— 0 callers, 6 callees
Verification — 870 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: 870 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
329 of 329 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— full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.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_blade_extractor.py— full-run-safetytests/test_build.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_cache.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_cluster.py— full-run-safetytests/test_cluster_exclude_hubs.py— full-run-safetytests/test_cobol_extractor.py— full-run-safetytests/test_codebuddy.py— 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_method_declarations.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-safety- … and 279 more
non-code file(s) changed (
graphify/skill-agents.md,graphify/skill-amp.md,graphify/skill-claw.md,graphify/skill-codex.md,graphify/skill-copilot.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-amp.md,graphify/skill-claw.md,graphify/skill-codex.md,graphify/skill-copilot.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.
· 1 more finding(s) on lines outside this diff (see the check run).
|
Thanks @venkatpachala. The prose cleanup is good — removing the duplicated "no other API keys are read" paragraph and the over-mandating language is worth keeping. But one part now conflicts with what shipped: this PR documents and tests "skip Part B for a fully cached semantic corpus / skip to Part C directly". That exact skip is what caused the #4116 Could you rebase on current |
Respect documented workflow skips and make semantic dispatch conditional on uncached host-agent work. Consolidate credential guidance while distinguishing the current skill route from the multi-provider CLI. Regenerate the 14 affected platform skills and their snapshots. Add 28 regression cases covering the shared-core platforms. Provider routing remains unchanged and is handled separately by Graphify-Labs#3255. Refs Graphify-Labs#4061. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Remove the fully cached Part B skip claim and related assertions while preserving the v0.9.77 Step B3 merge. Retain the credential and dispatch prose simplifications. Refs Graphify-Labs#4061 and Graphify-Labs#4110.
66a5ee1 to
8c678a6
Compare
|
@safishamsi Thanks for catching that. I rebased onto v8 at v0.9.77 and removed the fully cached Part B skip claim and related assertions. The credential and dispatch prose simplifications remain, while upstream’s Step B0 cleanup and Step B3 merge are preserved unchanged. |
What does this PR do?
The generated skills require every step and mandate subagents even though the workflow explicitly documents conditional skips. They also duplicate credential guidance and incorrectly describe Graphify as reading only Gemini keys.
This change simplifies the credential and dispatch guidance while accurately distinguishing the existing skill route from the headless CLI. The nearby parallel-start instruction respects the existing fast paths and cache check, and the introduction permits only explicitly documented skips. Source edits are confined to the shared core fragment and regression tests; 14 platform skills and their snapshots are regenerated.
Rebased on v8 at
5c7b847(v0.9.77). The fully cached Part B skip claim and related assertions have been removed. The upstream all-cached routing, Step B0 stale-chunk cleanup, Step B3 merge, and behavioural regression tests are preserved unchanged: cached semantic results still produce the input consumed by Part C.Fixes #4061.
Provider routing is unchanged. #3255 / #2513 owns that behavior change; these PRs share the credential paragraph and will need reconciliation if #3255 lands first.
Type of change
Verification & Invariants
The ordered workflow must permit only explicitly documented skips. The credential note must not ask for or block on a key, and dispatch guidance must account for host capabilities and the existing Gemini route. A fully cached semantic corpus still runs Step B3; an empty semantic file is appropriate only for the existing code-only fast path.
How was this tested?
Windows, Python 3.13.14; dependencies installed with
uv sync --all-extras --frozen.The above
python,ruff,pre-commit, andpyrightexecutables are from the project virtual environment./bin/bash; the same cases fail in the unchanged-v8 full run. Shell execution coverage is limited by that environment failure.Limitations: this is not an all-green full-suite run. Skip counts differ between checkouts; active collection skip marks are identical, but runtime skip reasons were not recorded, so identical coverage is not claimed. Windows pytest stalls while creating its optional current-directory symlink aliases; an external runner disables only
_pytest.pathlib._force_symlinkin each worker. Application symlink operations, repository test fixtures, and assertions are unchanged. The runner and pytest-xdist are local validation aids and are not included in this PR. Other Python versions and the Ubuntu CI matrix have not been run locally. These tests guard the instruction text and generated output; they do not measure every LLM's adherence or verify the pre-existing performance estimate.Graphify-specific checklist
--bless;--checkpasses.