Skip to content

fix: honor Claude CLI model env during labeling - #3876

Open
oleksii-tumanov wants to merge 1 commit into
Graphify-Labs:v8from
oleksii-tumanov:fix/claude-cli-label-model-env
Open

oleksii-tumanov wants to merge 1 commit into
Graphify-Labs:v8from
oleksii-tumanov:fix/claude-cli-label-model-env

Conversation

@oleksii-tumanov

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #3846.

GRAPHIFY_CLAUDE_CLI_MODEL already controls the Claude CLI model used for
semantic extraction, but community labeling and the dedup tiebreaker use the
separate _call_llm path. That path ignored the environment setting unless a
caller passed an explicit model, so label and cluster-only could silently use
the Claude CLI default instead.

This change applies the existing precedence at the plain-completion adapter:
an explicit model wins, otherwise a nonempty trimmed environment value is
forwarded, and no --model flag is added when neither is configured. Existing
explicit empty-string behavior is preserved.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Tests or CI
  • Refactor
  • Security fix

Verification & Invariants

The invariant is that both Claude CLI dispatch paths honor the same configured
model without replacing an explicit caller choice or changing the CLI default
when the setting is absent. The new argv matrix proves those boundaries, and a
label-batch regression proves the environment-only value reaches the real
labeling call chain while valid labels are still parsed. No graph, cache, or
generated format changes.

  • Read the CONTRIBUTING.md guide.
  • Reproduced the issue and identified the invariant.
  • Made the smallest fix necessary.
  • Added a regression test (if bug fix) or isolated boundary test.
  • Kept the PR description synchronized with the final implementation.
  • Documented any limitations / unsupported cases explicitly.

How was this tested?

python -m pytest -q tests/test_claude_cli_backend.py
46 passed

python -m pytest -q tests/test_claude_cli_backend.py tests/test_llm_parser.py tests/test_llm_parser_reasoning.py tests/test_llm_backends.py tests/test_label_retry.py tests/test_charmap_encoding.py
187 passed

ruff check .
All checks passed!

pyright tests/test_claude_cli_backend.py
0 errors, 0 warnings, 0 informations

python -m tools.skillgen --check
python -m tools.skillgen --audit-coverage
python -m tools.skillgen --schema-singleton
python -m tools.skillgen --monolith-roundtrip
python -m tools.skillgen --always-on-roundtrip
All five checks passed.

graphify update .
Completed AST-only.

The tests mock the Claude executable, so no credential, paid request, or live
model call was used. A fresh all-extras environment and full suite were not
available because dependency downloads timed out; the focused and affected
suites above ran against this checkout with the available local test
environment.

Graphify-specific checklist

  • I updated generated skill artifacts (uv run python -m tools.skillgen --bless) when changing their source fragments. (No source fragments changed.)
  • I confirmed that AST/structural extraction remains deterministic (no ambient state dependencies like ENV variables). (No AST path changed.)
  • I reviewed changes for security implications (no unsafe interpolation into shell/Python).
  • I confirmed no API keys or local-only graph data are included.
  • (If applicable) I disclosed AI authorship in my commit messages.

Use GRAPHIFY_CLAUDE_CLI_MODEL for plain Claude CLI completions when callers do not pass an explicit model. Preserve explicit-model precedence and the CLI default when the setting is absent.

AI assistance: OpenAI Codex was used to help implement and validate this change.
@github-actions

Copy link
Copy Markdown

Thanks for the pull request, @oleksii-tumanov. 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.

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).


Graphify review — findings

Lets the plain Claude CLI backend pick up a model from GRAPHIFY_CLAUDE_CLI_MODEL when no explicit model is passed to _call_llm, while an explicit model still wins and a blank/whitespace env value falls back to omitting --model entirely. Adds parametrized coverage for the precedence rules and confirms the label batch chain forwards env-only model selection.

No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 880 functions depend on the 239 functions this change touches.

Health — this change adds coupling hotspots:

  • new: deduplicate_entities() — 79 callers, 24 callees
  • new: build_merge() — 76 callers, 14 callees
  • new: extract_files_direct() — 17 callers, 20 callees
  • new: build() — 52 callers, 6 callees
  • new: _call_claude_cli() — 33 callers, 9 callees
  • new: main() — 98 callers, 3 callees
  • new: extract_corpus_parallel() — 26 callers, 11 callees
  • new: dispatch_command() — 2 callers, 125 callees
  • …and 18 more — each is listed as a finding

Verification — 880 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: 543 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

41 of 302 test file(s) selected (14%) via static blast radius.

  • tests/test_backend_env_isolation.py — impact
  • tests/test_backend_extras.py — impact
  • tests/test_build.py — impact
  • tests/test_build_merge_dedup_scope.py — impact
  • tests/test_build_merge_hyperedges_and_prune.py — impact
  • tests/test_build_merge_shrink_guard.py — impact
  • tests/test_carried_hyperedge_remap.py — impact
  • tests/test_charmap_encoding.py — impact
  • tests/test_chunking.py — impact
  • tests/test_claude_cli_backend.py — impact, changed-test
  • tests/test_corrupt_graph_json.py — impact
  • tests/test_cross_extension_reexport_self_cycle.py — impact
  • tests/test_dedup.py — impact
  • tests/test_dedup_remaps_hyperedges.py — impact
  • tests/test_dedup_survivor_richness.py — impact
  • tests/test_evidence_binding.py — impact
  • tests/test_file_slice.py — impact
  • tests/test_global_graph.py — impact
  • tests/test_go_qualified_resolution.py — impact
  • tests/test_hyperedge_member_shapes.py — impact
  • tests/test_image_vision.py — impact
  • tests/test_injection_sentinel_coverage.py — impact
  • tests/test_issue_3472_source_file_collision.py — impact
  • tests/test_label_retry.py — impact
  • tests/test_labeling.py — impact
  • tests/test_llm_backends.py — impact
  • tests/test_llm_parser.py — impact
  • tests/test_llm_parser_reasoning.py — impact
  • tests/test_no_dedup_flag.py — impact
  • tests/test_non_string_node_ids.py — impact
  • tests/test_ollama.py — impact
  • tests/test_ollama_retry_cap.py — impact
  • tests/test_oversized_document_slicing.py — impact
  • tests/test_partial_cache.py — impact
  • tests/test_pdf_slicing.py — impact
  • tests/test_pdf_token_estimate.py — impact
  • tests/test_provider_registry.py — impact
  • tests/test_prs.py — impact
  • tests/test_prune_sweeps_orphans.py — impact
  • tests/test_semantic_fragment_sanitize.py — impact
  • tests/test_unverified_semantic_shrink.py — impact

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.

· 26 more finding(s) on lines outside this diff (see the check run).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

claude-cli: community labeling ignores GRAPHIFY_CLAUDE_CLI_MODEL, only semantic extraction reads it

1 participant