Skip to content

fix(install): stop corrupting line endings and fix the re-install guard in _register_always_on_block - #3741

Closed
ayushcodes10 wants to merge 3 commits into
Graphify-Labs:v8from
ayushcodes10:fix-3668-install-crlf-and-idempotency-guard
Closed

ayushcodes10 wants to merge 3 commits into
Graphify-Labs:v8from
ayushcodes10:fix-3668-install-crlf-and-idempotency-guard

Conversation

@ayushcodes10

Copy link
Copy Markdown
Contributor

Fixes #3668.

_register_always_on_block (used by the claude/codebuddy install paths) had three related problems:

  1. Path.write_text opens in text mode, so on Windows appending a few lines to an existing CLAUDE.md rewrote the whole file to CRLF — an 85-line diff for a 3-line registration, for anyone keeping that file in a dotfiles repo or syncing it across platforms.
  2. The re-install guard was a bare "graphify" in content substring check — a file that merely mentions the project for any other reason gets silently treated as already registered, and a stale/edited block can never be refreshed by re-running install since the word is still present either way.
  3. _skill_registration emitted # graphify (H1), inconsistent with the ## graphify (H2) the *_MD_MARKER constants elsewhere in the file use.

I initially tried the issue's own suggested fix for #2/#3 — switch to ## graphify and reuse _replace_or_append_section/_CLAUDE_MD_MARKER directly, matching claude_install/gemini_install. That broke two existing tests (test_claude_subcommand_project_install_and_uninstall_are_project_scoped, test_uninstall_project_without_platform_removes_project_installs): _remove_claude_skill_registration deliberately matches the H1 # graphify heading specifically so it can never collide with a genuine user-authored H2 ## graphify heading elsewhere in the file (#2062). Switching to H2 would have quietly reverted that protection.

Instead: kept the H1 heading, and generalized _replace_or_append_section with a boundary_prefix parameter (mirroring the one _remove_marker_section already has), added a _SKILL_REGISTRATION_MARKER = "# graphify" constant as the single source of truth, and wired both _register_always_on_block (write) and _remove_claude_skill_registration (removal) to it so insert and removal can never drift apart again. CRLF fixed by passing newline="" on every write on this path, including gemini_install's GEMINI.md write, which the issue explicitly flagged as the same bug.

Five regression tests: an unrelated mention of "graphify" no longer suppresses the real block; a stale/edited block gets refreshed on re-install; both the CLAUDE.md and GEMINI.md writers now pass newline="" (verified via a call-tracking monkeypatch, since the actual CRLF corruption is only observable on Windows). All four bug-specific tests checked to fail against pre-fix code first. Full install test suite (213 tests) and the full suite (5718 passed) are green.

🤖 Generated with Claude Code

https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh

ayushcodes10 and others added 3 commits September 22, 2026 14:58
_register_always_on_block had three problems, all only visible on the
claude and codebuddy paths that call it. Path.write_text opens in
text mode, so on Windows appending a few lines to an existing
CLAUDE.md rewrote the WHOLE file to CRLF, turning a one line
registration into an eighty five line diff for anyone who keeps that
file in a dotfiles repo or syncs it across platforms. Separately, the
re install guard was a bare check for the word graphify anywhere in
the file, so a user whose file mentioned the project for any other
reason got silently treated as already registered and never received
the real block, and an edited or stale block could never be repaired
by re running install since the word was still present either way.

Switch the write to pass newline equals empty string so existing line
endings are never translated, and switch the guard to
_replace_or_append_section, the same marker anchored helper
claude_install and gemini_install already use, so a stale block is
correctly refreshed instead of silently skipped. That helper's
boundary detection was hardcoded to the next H2 heading, which does
not fit this block's own heading level, so it gains a boundary_prefix
parameter mirroring the one _remove_marker_section already has, kept
in sync with a new shared marker constant so the paired removal
function matches on exactly the same heading. Deliberately kept at H1
rather than switched to the H2 markers the issue itself suggested,
since the existing removal function anchors on H1 specifically to
never collide with a genuine user authored H2 heading elsewhere in
the same file, a design choice from a separate earlier issue that a
plain heading level change would have quietly broken.

gemini_install writes GEMINI.md through the exact same text mode
write, so it gets the same newline fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Cover an unrelated mention of the word graphify no longer suppressing
the real registration block, a stale or hand edited block correctly
getting refreshed on re install instead of being left alone, and both
the CLAUDE.md and GEMINI.md writers now passing newline equals empty
string so an existing file's own line endings are never translated on
write.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh

@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.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.


Graphify review — findings

Fixes graphify install on Windows so appending its registration to an existing CLAUDE.md/GEMINI.md no longer rewrites the whole file to CRLF, by writing every shared file with newline="". Replaces the bare "graphify" in content idempotency check in _register_always_on_block with a marker-anchored _replace_or_append_section keyed on the new H1 _SKILL_REGISTRATION_MARKER, so an unrelated mention of the word no longer suppresses the real block and a stale or hand-edited block gets refreshed on re-install instead of skipped. Extends _replace_or_append_section with a boundary_prefix argument so the H1 skill-registration section matches its removal counterpart on where the section ends.

Worth a look

  • _skill_registration lost leading newline, no separation from prior content on appendgraphify/install.py:401 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • CRLF files are normalized to LF before appending registrationgraphify/install.py:397 · 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 — 752 functions depend on the 520 functions this change touches.

Health — this change adds coupling hotspots:

  • new: dispatch_command() — 2 callers, 125 callees
  • new: install() — 13 callers, 11 callees
  • new: codebuddy_install() — 20 callers, 5 callees
  • new: gemini_install() — 11 callers, 7 callees
  • new: claude_install() — 19 callers, 4 callees
  • new: _copy_skill_file() — 12 callers, 6 callees
  • new: claude_uninstall() — 17 callers, 4 callees
  • new: _project_uninstall() — 5 callers, 13 callees
  • …and 14 more — each is listed as a finding

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

Test selection

Test selection

290 of 290 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-safety
  • tests/test_affected_member_seed.py — full-run-safety
  • tests/test_agents_platform.py — impact, full-run-safety
  • tests/test_analyze.py — full-run-safety
  • tests/test_anthropic_custom_endpoint.py — full-run-safety
  • tests/test_antigravity_install.py — full-run-safety
  • tests/test_apm_fallback_version.py — full-run-safety
  • tests/test_architecture_doc.py — full-run-safety
  • tests/test_astro_extraction.py — full-run-safety
  • tests/test_astro_import_ids.py — full-run-safety
  • tests/test_atomic_canvas_export.py — full-run-safety
  • tests/test_atomic_version_stamp.py — impact, full-run-safety
  • tests/test_atomic_writes.py — full-run-safety
  • tests/test_backend_env_isolation.py — full-run-safety
  • tests/test_backend_extras.py — full-run-safety
  • tests/test_benchmark.py — full-run-safety
  • tests/test_benchmark_raw_graph.py — full-run-safety
  • tests/test_build.py — full-run-safety
  • tests/test_build_merge_dedup_scope.py — full-run-safety
  • tests/test_build_merge_hyperedges_and_prune.py — full-run-safety
  • tests/test_build_merge_shrink_guard.py — full-run-safety
  • tests/test_builtin_global_type_refs.py — full-run-safety
  • tests/test_cache.py — full-run-safety
  • tests/test_callflow_html.py — full-run-safety
  • tests/test_cargo_introspect.py — full-run-safety
  • tests/test_carried_hyperedge_remap.py — full-run-safety
  • tests/test_case_sensitive_resolution.py — full-run-safety
  • tests/test_charmap_encoding.py — full-run-safety
  • tests/test_chunking.py — full-run-safety
  • tests/test_cjs_module_extension.py — full-run-safety
  • tests/test_claude_cli_backend.py — full-run-safety
  • tests/test_claude_md.py — impact, full-run-safety
  • tests/test_cli_broken_pipe.py — full-run-safety
  • tests/test_cli_export.py — full-run-safety
  • tests/test_cli_help.py — full-run-safety
  • tests/test_cluster.py — full-run-safety
  • tests/test_codebuddy.py — impact, full-run-safety
  • tests/test_community_hub_labels.py — full-run-safety
  • tests/test_community_labels_skill.py — full-run-safety
  • tests/test_confidence.py — full-run-safety
  • tests/test_corrupt_graph_json.py — full-run-safety
  • tests/test_cpp_nested_and_cli.py — full-run-safety
  • tests/test_cpp_objc_cross_file_calls.py — full-run-safety
  • tests/test_cpp_preprocess.py — full-run-safety
  • tests/test_cross_extension_reexport_self_cycle.py — full-run-safety
  • tests/test_cross_language_call_resolution.py — full-run-safety
  • tests/test_cross_repo_external_call_guards.py — full-run-safety
  • tests/test_cross_repo_member_calls.py — full-run-safety
  • tests/test_cross_repo_shared_types.py — full-run-safety
  • tests/test_csharp_call_site_generic_args.py — full-run-safety
  • … and 240 more

non-code file(s) changed (CHANGELOG.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 (CHANGELOG.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.

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

@safishamsi

Copy link
Copy Markdown
Collaborator

Shipped in v0.9.66 (on PyPI). Cherry-picked with authorship preserved so it shows under your GitHub contributions. Thanks @ayushcodes10!

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

Labels

None yet

Projects

None yet

2 participants