Skip to content

fix(update): detect managed-EOL churn from --numstat, not --name-only - #75213

Closed
Zeraphim wants to merge 3 commits into
NousResearch:mainfrom
Zeraphim:fix/75175-managed-eol-crlf
Closed

fix(update): detect managed-EOL churn from --numstat, not --name-only#75213
Zeraphim wants to merge 3 commits into
NousResearch:mainfrom
Zeraphim:fix/75175-managed-eol-crlf

Conversation

@Zeraphim

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a broken line-ending normalization path in the managed-checkout updater. hermes update is supposed to take a Git-for-Windows-managed clone (pinned core.autocrlf=true, CRLF working tree) off autocrlf and rewrite the churn back to LF before persisting the core.autocrlf=false pin. The "churn that is only CRLF" set was computed via git diff --name-only --ignore-cr-at-eol, but whitespace-ignore flags are inert under --name-only, so eol_only was always empty, the cleanup checkout never ran, and the pin was written unconditionally — handing the update a whole-tree autostash of CRLF churn (the regression that made the merged PR #74487 fail its own 5-test cluster).

_eol_only() now derives the "files that still differ after ignoring CRLF-at-EOL" set from git diff -z --ignore-cr-at-eol --numstat (which honors the flag and omits CRLF-only files) instead of the inert --name-only listing. Both call sites — the initial detection and the post-checkout recheck — use the corrected computation, so the whole bug class (detection + verification) is fixed.

Related Issue

Fixes #75175

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/update_cmd.py: added _real_dirty() reading --numstat to honor --ignore-cr-at-eol; _eol_only() now computes all_dirty - real_dirty with a working real-diff set. No change to the checkout/recheck/pin flow or fail-safes.

How to Test

  1. Baseline (before fix): scripts/run_tests.sh tests/hermes_cli/test_update_eol_churn.py -v → 5 failures (test_churn_invisible_under_autocrlf_true_is_still_found, test_churn_is_cleared_and_the_pin_is_persisted, test_real_edits_survive_even_when_line_endings_also_flipped, test_pin_is_withheld_when_the_churn_cannot_be_cleared, test_churn_across_more_files_than_fit_in_one_argv).
  2. After fix: same command → 9/9 pass.
  3. Regression sweep: scripts/run_tests.sh over the 19-file update family → 114/114 pass.
  4. Root-cause check: git diff --name-only --ignore-cr-at-eol lists CRLF-only files (flag inert); git diff --numstat --ignore-cr-at-eol omits them (flag honored).

Checklist

Code

Documentation & Housekeeping

  • N/A (no user-facing docs changes)
  • N/A (no config keys)
  • N/A (no architecture/workflow changes)
  • N/A (cross-platform: --numstat -z is ancient; --ignore-cr-at-eol requires git >= 2.35, which the prior code already relied on)
  • N/A (no tool schemas)

@Zeraphim
Zeraphim marked this pull request as ready for review July 31, 2026 04:51
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 31, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating the --name-only --ignore-cr-at-eol path; current main still has that exact call at hermes_cli/update_cmd.py:3087.

Problems

  • hermes_cli/update_cmd.py:3099 assumes every git diff --numstat -z item is added\tdeleted\tpath. Git’s documented rename/copy form is added\tdeleted\tNUL preimage NUL postimage NUL. With rename detection enabled, this comprehension adds only the empty field and omits the postimage. Since --name-only reports the postimage path, _eol_only() can mark a real renamed/copied edit as EOL-only and send it to checkout.

Suggested changes

  • Decode the NUL-framed numstat stream, recognizing the rename/copy marker and retaining the postimage path; add a regression case with rename detection enabled.

Automated hermes-sweeper review.

Comment thread hermes_cli/update_cmd.py Outdated
@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 31, 2026
@Zeraphim

Copy link
Copy Markdown
Contributor Author

Hello @teknium1, addressed on the latest head 2846789:

  • Replaced the flat --numstat -z comprehension with a fail-closed parser that recognizes Git's NUL-framed rename/copy form and retains the postimage path used by --name-only.
  • Added an integration regression with rename detection and an intent-to-add postimage. Before the fix, managed-EOL cleanup checked out the misclassified postimage as an empty file and discarded the real edit; the test now verifies that the renamed edit survives and the checkout is pinned successfully.
  • Merged the latest canonical main into the PR branch without conflicts.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Three PRs address Issue #75175 by replacing the ineffective --name-only --ignore-cr-at-eol comparison with a --numstat-based real-edit probe. #75213 additionally handles NUL-framed rename/copy records and adds a preservation regression test, #75220 contains an unsafe incomplete parser, and merged #76431 implements the root-cause fix with rename detection disabled for its plain-diff path.

Related pull requests

Duplicates

#75213, #75220, and #76431 implement the same root fix of replacing the ineffective ignored-EOL name-only probe with numstat; #75220 is the incomplete duplicate, while #75213 adds defensive NUL-framed rename/copy parsing and a regression test beyond merged #76431.

Suggested consolidation

Close #75213 as already implemented on main by merged #76431, and retain #75220 as closed as a duplicate of #75213/#76431. The concrete evidence is the merged #76431 diff in hermes_cli/update_cmd.py, which replaces _dirty("--ignore-cr-at-eol") with a --numstat --ignore-cr-at-eol real-dirty probe; its discussion records CI on head 7e89ed9c01368cdc0711370cb9ecc772d7145b0d. Despite the keep_open review on #75213, the displayed #75213 head already addresses that review's rename/copy concern, while merged #76431 removes the reported root cause through a plain-diff path where its own diff documents that rename detection is off.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I75175(["issue #75175 (open)"])
    subgraph Dup75213 ["PRs duplicating each other"]
        P75213["PR #75213 (open)"]
        P75220["PR #75220 (closed)"]
        P76431["PR #76431 (merged)"]
    end
    P75213 -->|best fix| I75175
    class I75175 open
    class P75213 open
    class P75220 closed
    class P76431 merged
    class P75213 best
    class P75213 target
    click I75175 "https://github.com/NousResearch/hermes-agent/issues/75175"
    click P75213 "https://github.com/NousResearch/hermes-agent/pull/75213"
    click P75220 "https://github.com/NousResearch/hermes-agent/pull/75220"
    click P76431 "https://github.com/NousResearch/hermes-agent/pull/76431"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 7 kB of PR diffs, 15 kB of issue/PR text, 5 kB of discussion (9 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@Zeraphim Zeraphim closed this Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_normalize_managed_eol does not clear CRLF churn — 5-test cluster fails (merged PR #74487 fails its own tests)

4 participants