Skip to content

fix(check-new-line-breaks): scope from the working tree when dirty and report the examined count (#825) - #826

Merged
d-morrison merged 7 commits into
mainfrom
fix/825-nlb-dirty-tree
Sep 3, 2026
Merged

fix(check-new-line-breaks): scope from the working tree when dirty and report the examined count (#825)#826
d-morrison merged 7 commits into
mainfrom
fix/825-nlb-dirty-tree

Conversation

@d-morrison

@d-morrison d-morrison commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Closes #825.

check-new-line-breaks.py scoped its check with git diff <base>...HEAD (committed only) and then read line content from the working tree, so a run before committing reported clean about lines it never examined. Recorded five times in Morrison-Lab/ai-config (latest ai-config#2542, which carries the reproducer).

What changes

  • Working-tree-aware scope. NLB_SCOPE=auto|worktree|committed (default auto). In auto, a dirty tree (tracked changes, staged or unstaged, that the globs match and the ignore patterns do not exclude) scopes the diff from the explicit merge base against the working tree; a clean tree keeps the committed-only scope CI relies on. The merge base is resolved once with git merge-base, so a stale base does not widen the diff in either scope.
  • Untracked files cannot be seen by git diff, so they do not widen the scope; the run prints a ::warning:: naming untracked files that match the globs and telling the user to git add them.
  • Search-space line. Examined N added line(s) across M file(s) (scope: committed|working tree). precedes the verdict, so a run that examined nothing is visibly different from a clean run.
  • Docstrings state that line content is always read from the working tree, so forcing committed on a dirty tree can desync; CLAUDE.md's passage on the local run is rewritten to describe the fix rather than the limitation.

Tests

check-new-line-breaks/tests/: 99 to 110 passing. New cases cover the uncommitted violation (with the committed case as control), staged-but-uncommitted, untracked (warns, does not widen), clean dirty tree and clean committed tree examined counts, the empty diff, explicit NLB_SCOPE=committed on a dirty tree, and the merge-base anchor under an advancing base in both scopes, each with a naive-diff sibling assertion proving the control would flag without the anchor.

The ai-config#2542 reproducer now exits 1 on the uncommitted state and 1 on the committed control.

Review provenance

Implemented by a sonnet subagent; two agy --print adversarial rounds (seven implementation findings, then three test-quality findings), all addressed. Pushed with ALLOW_UNREVIEWED_PUSH=1 because those reviews came from a separate CLI rather than a subagent, one of the guard's named override cases.

Consumer follow-up: bump scripts/vendor/gha-check-new-line-breaks.pin in ai-config once this merges.

🤖 Generated with Claude Code

d-morrison and others added 4 commits September 2, 2026 19:38
…d count)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…eport the search space

A local run before committing diffed only `<base>...HEAD` (committed-only)
while reading line content from the working tree, so it silently examined
zero lines and printed the same "No lines missing semantic breaks." a
genuine pass would. `_added_line_numbers` now widens to the working tree
(including staged content) whenever it or the index carries a change to a
matched file, controlled by `NLB_SCOPE` (default `auto`; also `worktree` and
`committed`), and every run now prints how many added lines and files it
examined and under which scope. The merge base of `base_ref` and `HEAD` is
resolved once, explicitly, via `git merge-base`, shared by both scopes, so a
base branch that has advanced since divergence still does not widen what
gets checked -- unchanged from the prior three-dot semantics.

Adds six pytest cases: an uncommitted violation flagged under `auto` scope,
the same bytes committed as the control, a clean dirty tree and a clean
committed tree each reporting the examined count, `NLB_SCOPE=committed`
forcing the old behavior over a dirty tree, and a base branch advancing
after divergence still not flagging the pinned merge-base anchor.

Verified against the exact reproducer in ai-config#2542: the uncommitted
case now exits 1 (previously 0), matching the committed control.

Fixes #825. Ported from ai-config#2542.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…fa673

1. An untracked new file no longer flips "auto" scope to "worktree" on its
   own -- `_has_uncommitted_changes` now passes `-uno` to `git status`, so
   plain `git diff`'s inability to see untracked content can no longer be
   silently masked by a scope that claims to have widened. A separate
   `_untracked_matches` helper names any untracked file matching the globs
   in an explicit `::warning::` line instead.
2. `_has_uncommitted_changes` and `_untracked_matches` both now take
   `ignores` and filter each `git status --porcelain` path through
   `_ignored`, so an uncommitted edit confined to an ignored file no longer
   widens scope for everything else.
3. `_merge_base` now treats empty or whitespace-only `git merge-base`
   output as unresolved (returns None), not just an outright failure.
4. Reworded the `NLB_SCOPE`/`_added_line_numbers` docstrings to state the
   actual behavior: line *content* is always read from the working tree
   regardless of scope, so forcing `NLB_SCOPE=committed` on a dirty tree can
   desync reported line numbers -- it is for reproducing CI's own behavior
   on a clean tree, not for selectively ignoring uncommitted changes.
5. `examined_files` now only counts a file that contributed at least one
   examined line, so a file whose added lines are all non-prose no longer
   prints a misleading "0 added line(s) across 1 file(s)".
6. Tests: rewrote the committed-scope test to use a base with one committed
   clean line plus an uncommitted violation, so it actually distinguishes
   "committed" from "worktree"/"auto" (with a companion asserting "auto"
   catches what "committed" misses); added a dirty-tree companion to the
   merge-base-advancing test; added a staged-but-uncommitted case; added an
   untracked-file case asserting both the no-widen behavior and the
   warning; added an empty-diff case asserting "Examined 0 ... (scope:
   committed)." and a pass.
7. Replaced the Unicode box-drawing dashes in the gha#825 test section
   header with ASCII dashes.

Suite: 105 -> 110 passed (`python3 -m pytest check-new-line-breaks/tests/ -q`).
py_compile and pyflakes clean on both files.

Refs #825.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…y regression-worthy

The two "base branch advancing" tests could not catch a regression that
drops the merge-base anchor: a violation ADDED on trunk after divergence
shows up as a DELETION from the feature branch's side of a direct
`git diff trunk`, never as an addition, so `_added_line_numbers` never
surfaces it either way -- the tests passed for the wrong reason.

Rebuilt both (committed and worktree-scope variants) so the violation
starts in the SHARED base commit, trunk rewrites it into a clean line, and
feature leaves the original untouched. A diff anchored directly at trunk's
later commit then reports the untouched violating line as an ADDITION
(trunk's side now differs), while the merge-base anchor (the shared base,
not trunk's rewrite) correctly excludes it. Added a `_find_naive` test
helper (monkeypatches `_merge_base` to return `base_ref`'s own SHA) and a
sibling assertion in each test proving that naive-anchor path DOES surface
the line -- so the real assertion is now known to exercise something a
regression could break.

The worktree-scope variant now takes `capsys` and asserts the exact
"Examined 2 added line(s) across 1 file(s) (scope: working tree)." line,
so a silent fall-through to committed scope cannot pass vacuously. Moved
the two tests adjacent (committed variant first) and had the worktree
variant's comment name the committed one explicitly, rather than "the test
above" pointing at a test several functions away.

Suite: still 110 passed (`python3 -m pytest check-new-line-breaks/tests/ -q`);
py_compile and pyflakes clean.

Refs #825.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@d-morrison
d-morrison marked this pull request as ready for review September 3, 2026 05:14
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

d-morrison and others added 2 commits September 2, 2026 22:47
…scope (#825)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…function-per-file

The script already carried 18 helpers before #825; the changed-file scan
flags it now that the diff touches it. The marker is the one the check names.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@d-morrison

Copy link
Copy Markdown
Collaborator Author

Round at 2fb827fb, addressing the review of a687dfbc and the one red check:

  • Missing changelog fragment --- Addressed. changelog.d/check-new-line-breaks-worktree-scope.fixed.md describes the NLB_SCOPE knob, the dirty-tree scope, the untracked-file warning, and the examined-count line, citing check-new-line-breaks: a diff-scoped run before committing cannot fail; scope from the working tree when dirty and report the examined count #825 and this PR.
  • one-function-per-file red --- Addressed. The script carried 18 helper functions before this PR and gained five more; the changed-file scan flags it now that the diff touches it. It now carries the # check-one-function-per-file: allow-multiple marker the check names, which is how check-one-function-per-file.py itself opts out. The whole-repo local run no longer lists this file.

Posted by Claude Code (AI agent) --- not written by a human.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

The single-line bullet tripped the semantic-line-break gate it documents.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@d-morrison

Copy link
Copy Markdown
Collaborator Author

Round at 6c18993b, addressing the review of 2fb827fb and the two red checks:

  • Changelog fragment packs several sentences on one line --- Addressed. The bullet is re-broken at clause boundaries like its sibling fragments. Both new-line-breaks and diff-scoped-guard were red on that same line, so one fix covers both.

Measured while fixing it: with the fragment edited but not yet committed, this branch's own checker reported Examined 56 added line(s) across 2 file(s) (scope: working tree). and then No lines missing semantic breaks., which is the behaviour #825 asks for; on main the same pre-commit run would have examined nothing.

Posted by Claude Code (AI agent) --- not written by a human.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


This matches what the prior rounds already reviewed — this round's actual delta is just the two commits in the computed range (the changelog fragment reflow, plus a merge commit). No other files changed.

The fix is verified correct: reflowing the fragment across ten lines at clause boundaries resolves the semantic-line-breaks violation flagged in the prior round, confirmed by re-running the exact reproduction command from that review (NLB_GLOBS='*.md *.qmd' NLB_SCOPE=committed NLB_BASE_REF=origin/main python3 check-new-line-breaks/check-new-line-breaks.py), which now reports "No lines missing semantic breaks." The full test suite still passes (110 passed). No new files or logic changed in this range beyond the fragment itself.

Review

Scope of this round: commits 6c18993 (reflow the #825 changelog fragment at clause boundaries) and the merge commit 1f4d2f8, closing the range 2fb827f..1f4d2f8. This addresses the prior round's sole finding — the fragment's self-inflicted new-line-breaks CI failure.

Verification performed:

  • Reproduced the prior round's failing command against the new fragment content: now passes clean (No lines missing semantic breaks, 47 lines examined across 2 files).
  • Confirmed the fragment's prose content is byte-identical in meaning to the pre-fix version — only line breaks changed, at clause/sentence boundaries, matching the style of sibling fragments (e.g. nlb-moved-content-exemption.fixed.md).
  • Re-ran python3 -m pytest check-new-line-breaks/tests/ -q → 110 passed, unchanged.
  • Confirmed no other file changed in this range (git diff --stat 2fb827f 1f4d2f8 touches only the fragment).

No new findings. This closes the last two rounds' issues in sequence: round 1 flagged the missing changelog fragment (fixed in c2ccbaa), round 2 flagged that fragment's own line-length violation (fixed in 6c18993, this round). Both fixes verified by direct reproduction rather than by inspection alone.

Verdict

Ready for merge — both prior findings are resolved and verified by reproduction; no new issues found in this round's diff.

Structured Review Data (JSON)
{
  "schema_version": "1.1",
  "reviewer": "claude",
  "commit_sha": "1f4d2f865797ad0620bed9d7f881fd1030417798",
  "verdict": "CLEAN",
  "findings": [],
  "detailed_assessment": "The changelog.d/check-new-line-breaks-worktree-scope.fixed.md fragment was reflowed at clause boundaries across 10 lines, verified to pass the new-line-breaks check via direct reproduction of the exact command that previously failed it.",
  "holistic_assessment": "Both prior rounds' findings (missing changelog fragment, then that fragment's own semantic-line-break violation) are now closed in sequence, the full test suite remains green at 110 passed, and no other file changed in this round's diff range."
}

Reviewed commit: 6c18993

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

💰 Cost: $0.7400 (review) — run

@d-morrison
d-morrison merged commit b91ecff into main Sep 3, 2026
67 checks passed
@d-morrison
d-morrison deleted the fix/825-nlb-dirty-tree branch September 3, 2026 06:27
d-morrison added a commit to Morrison-Lab/ai-config that referenced this pull request Sep 3, 2026
…b-checker --help, hook NLB_SCOPE (#3097)

* ums: gha one-function-per-file baseline, changelog NLB scope, sync-nlb-checker --help, hook NLB_SCOPE fix

Four learnings from driving Morrison-Lab/gha#826 (check-new-line-breaks
working-tree-scope fix) and #3089 (its vendored
pin bump), recorded in memories/gha-reusable-workflows.md and
shared/writing/semantic-line-breaks.md:

- gha's one-function-per-file check scans changed files only, so
  touching a pre-existing multi-function script (18 top-level defs)
  goes red; the fix is the check's own allow-multiple marker, not a
  split.
- changelog.d fragments are scanned by gha's new-line-breaks and
  diff-scoped-guard jobs like any other .md file, so they need the
  same clause-break treatment as their siblings.
- scripts/sync-nlb-checker.py takes no arguments and runs the sync on
  any invocation including --help; tracked as ai-config#3095.
- gha#826's NLB_SCOPE=auto widens the check to the working tree when
  dirty; the pre-push hook now pins NLB_SCOPE=committed, closing the
  ai-config#3027 "hook over-warns" question.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(gha memory): state the opt-out marker placement as measured, not as a rule

Review round on dc22c1b: gha's own check carries the marker after its
module docstring, while gha#826 placed it after the shebang; the check
accepts both, and its message says only "near the top".

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.

check-new-line-breaks: a diff-scoped run before committing cannot fail; scope from the working tree when dirty and report the examined count

1 participant