check-new-line-breaks: add clause-break check (SemBr rule 5), on by default - #337
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
…efault Flags a newly-added line that joins two independent clauses with a semicolon, alongside the rule 4 sentence check that already ran. On by default (clause-breaks: false opts out); safe because the check stays warn-only unless fail: true. Semicolon only, past a length gate: measured over a 21,963-line conformant corpus, keying on every mark the spec names flags 50.5% of already-conforming lines against 1.1% for this check. Closes #336
|
Verified the doc sync against the rendered preview, not just the source, since From
Both new rows render, and their defaults match One note on how that check was run, since it nearly went wrong: my first parse Generated by Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code reviewReviewed the Two findings posted inline on
One smaller, non-blocking item not worth an inline anchor: No CLAUDE.md structural/doc-sync violations found, and no syntax or import errors (
VerdictNeeds more work — finding 1 (the length-gate/stripped-text mismatch) is a real false-positive bug worth fixing before merge; finding 2 is a documentation-accuracy correction. Neither is large in scope. |
|
Claude finished review — View run Review posted on PR #337. Summary: Reviewed the
Also noted a minor, non-blocking off-by-one in the VerdictNeeds more work — the length-gate/stripped-text mismatch is a real bug worth fixing before merge. |
…op the em-dash aside Round 1 findings on #337: 1. has_unbroken_clause gated on len(text) while searching the stripped text, so a short line inflated past the gate by a link target or code span was flagged. Reproduced with the reviewer's own example (128 raw chars, 16 visible). Now gates on the stripped length, which also matches the URL-inflation exception in ai-config's semantic-line-breaks guidance. 2. The docstring's em-dash 'banned from source files' aside explained a code path that does not exist -- the predicate never looks for a dash -- so it is dropped rather than caveated. 3. clause-min-length is now inclusive, so the input's own name is true; that was cheaper than rewording five doc sites to say 'strictly greater than'. Three regression tests added, each confirmed to fail against the pre-fix gate. Re-measured every published figure against the current corpus, since the gate change moved them: 22,820 lines, 50.6% for all marks, 0.7% for this check (was 1.1%).
Code reviewSecond round. Both prior findings are fixed and now regression-tested: Five findings posted inline on this round's diff, all independently verified by executing the actual module or reading the actual files (not inferred):
One additional non-blocking note, not tied to a diff line so not posted inline: the PR description (not the code) still asserts " VerdictNeeds more work - finding 1 (bare-URL bypass) is a real, easily-reproduced false-positive bug worth fixing before merge, and finding 3 (no-op selftest step whose comment overstates its own coverage) misrepresents test coverage in both the workflow and the newly-added CLAUDE.md paragraph. Findings 2, 4, and 5 are smaller in scope. |
|
Claude finished review — View run Code reviewSecond round. Both prior findings are fixed and now regression-tested: Five findings posted inline on this round's diff, all independently verified by executing the actual module or reading the actual files (not inferred):
One additional non-blocking note, not tied to a diff line so not posted inline: the PR description (not the code) still asserts " VerdictNeeds more work - finding 1 (bare-URL bypass) is a real, easily-reproduced false-positive bug worth fixing before merge, and finding 3 (no-op selftest step whose comment overstates its own coverage) misrepresents test coverage in both the workflow and the newly-added CLAUDE.md paragraph. Findings 2, 4, and 5 are smaller in scope. |
Five findings on #337, all reproduced against the real module first: 1. Bare URLs and autolinks had no ']( ' to anchor on, so they bypassed strip_inline_markup entirely -- reintroducing round 1's URL-inflation bug for unbracketed links, and reading a ';' in a query string as a clause. 2. HTML entities end in ';' too, and were not stripped. 3. The 'opted out' selftest step sets no fail:, so it could not prove the input reaches the script; its comment and the matching CLAUDE.md paragraph both claimed it did. Added real env-var -> main() -> exit-code coverage and corrected both claims. 4. clause-min-length was type: number, against the repo's idiom for length-threshold inputs forwarded to a composite (all quoted strings). 5. A test named for find_violations only called classify_line; replaced by the exit-code tests above, which cover the plumbing it claimed to. strip_inline_markup now documents its invariant: it only ever removes characters, so visible length is a conservative lower bound and under-counting can only suppress a flag, never invent one. That is why entities are deleted rather than replaced with a placeholder. Six new tests, each confirmed to fail against the defect it guards. Re-measured the all-marks figure, which the extra stripping moved to 50.5%.
dem-extra1
changed the title
|
Working on this — paws off until I'm done. Self-review, for the same reason as on #348: What I verified rather than read58 tests pass. I also ran the The defaults really are pinned across all three sites. I expected to find that the reusable workflow's
Added lines are free of em-dashes, en-dashes, curly quotes, and Findings1. The title contradicts the implementation. The PR is titled "add opt-in clause-break mode", but the feature is opt-out — on by default, per the maintainer direction the body itself cites and the 2. The diff adds a link that will 404 after the org move. Things I checked and found correct
VerdictNeeds more work — two fixes, both one-liners: the title, and the Sequencing: same as #348 — merge #351 first, then Generated by Claude Code |
GitHub's repository-transfer redirect covers repository, tree, blob, and pull paths, but not issues paths, so a d-morrison/gha/issues/N URL now 404s rather than redirecting. Verified: /issues/336 returns 200 while the d-morrison form does not resolve. links / link-checker is already red on main over 17 such URLs; these two added lines would have contributed an eighteenth. #351 retargeted `uses:` references but deliberately left prose links alone, so these are not covered by the merge of main that this branch just took. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X4epyWdSsXs88wgDNTaKvc
dem-extra1
changed the title
|
Both findings from my review above are Addressed. Pushed 1. Title — fixed. Now "check-new-line-breaks: add clause-break check (SemBr rule 5), on by default", so the squash-merge commit will not describe the behavior as opt-in when it ships opt-out. 2. The One correction to my own review while I'm here: I wrote that the URL was in
Those are the two I changed. Re-ran the suite after the change: 58 passed. Worth noting the merge of Still expect Generated by Claude Code |
| **A selftest step that sets no `fail:` cannot prove the input reached the | ||
| script, however it is worded.** | ||
| `_selftest.yml`'s `new-line-breaks` job does call the composite a second time | ||
| with `clause-breaks: 'false'`, and that is worth having as a real `uses:` | ||
| exercise -- but `main()` returns 0 on every path unless `NLB_FAIL` is set, so | ||
| the step stays green whether the input arrives, is dropped, or was never | ||
| declared at all (an undeclared composite input is only an Actions warning). | ||
| What actually pins the `env var -> main() -> exit code` path is a set of | ||
| pytest cases that set `NLB_FAIL=true` around a real `main()` call on a | ||
| throwaway git repo, asserting exit 1 with the clause check on and exit 0 both | ||
| with `NLB_CLAUSE_BREAKS=false` and with the length gate raised past the line. | ||
| Each was confirmed to fail when the corresponding env read is stubbed out. | ||
| (gha#337 review round 2: the step's original comment, and this paragraph, | ||
| both claimed the step proved the plumbing; neither could.) | ||
| Round 3 added the converse caveat, since "cannot prove the input arrived" is | ||
| not "proves nothing": the step still pins that `action.yml` parses with the | ||
| input declared and that the opt-out code path runs to completion, which is | ||
| why it stayed rather than being deleted as dead weight. |
There was a problem hiding this comment.
Self-contradiction: this paragraph both asserts and refutes the same claim about what the selftest step proves.
Line 415 correctly states "an undeclared composite input is only an Actions warning" — meaning a uses: step with an undeclared with: key still goes green. But the round-3 addition at the end of the same paragraph (lines 423-425) claims: "the step still pins that action.yml parses with the input declared and that the opt-out code path runs to completion, which is why it stayed rather than being deleted as dead weight."
Since the step can't fail on an undeclared input (per line 415's own accurate statement) and main() always exits 0 unless NLB_FAIL is set, the step's green status is identical whether or not clause-breaks is actually declared in action.yml. So it does not "pin" declaration — only line 415's caveat is true; the round-3 addition's "still pins ... declared" clause isn't.
This is the same finding as the paired one on .github/workflows/_selftest.yml's step comment, which makes the identical claim — both need the same correction.
Suggested fix: rewrite the round-3 sentence to drop the "parses with the input declared" claim, e.g.: "Round 3 added the converse caveat, since 'cannot prove the input arrived' is not 'proves nothing': the step still pins that the opt-out code path runs to completion without an unhandled exception, which is worth keeping even though it can't show the input is declared."
Ref:
Lines 408 to 425 in 0bde33e
Code reviewReviewed the current diff (head Four new findings from this round, posted inline:
All four are backed by direct execution/verification against the current source, not just re-reads of the prior review threads. VerdictNeeds more work — two of the four findings (leading-semicolon whitespace bug, |
|
Claude finished review — View run Posted successfully. Summary comment: #337 (comment) Review summary for #337Verified that all findings from prior review rounds (length-gate measurement, em-dash doc claim, bare-URL/autolink stripping, HTML entities, the un-provable selftest step wording, Four new, independently-verified findings posted as inline comments:
VerdictNeeds more work — two real correctness/robustness bugs in the detection logic plus two documentation-accuracy issues. |
1. has_late_semicolon: strip() rather than rstrip(). Stripping a construct can leave whitespace at the line start, so a semicolon separated from it by one space was reported as an interior clause break while the identical no-space line was correctly ignored. 2. _env_int: warn on an unparseable value instead of falling back silently. The round-3 note claimed both readers had been fixed; only _env_flag and _env_int's negative branch were. Unset stays silent, which is the normal case. 3. changelog fragment cites #336 as a bare ref, matching the (#328)/gha#253 form the other fragments use and auto-linking when CHANGELOG.md renders. 4. _selftest.yml and CLAUDE.md no longer claim the opt-out step pins that the input is declared -- an undeclared composite input is only a warning, as the same CLAUDE.md paragraph already said two sentences earlier. Point at the defaults-agreement test, which does pin it. Regression tests added for 1 and 2, each confirmed to fail against the defect it guards before being kept, plus a guard that an unset variable stays silent. 62 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X4epyWdSsXs88wgDNTaKvc
|
Round 5 — all four findings dispositioned. Pushed 1. Leading-semicolon guard defeated by a stray space — AddressedCorrect, and a real detection bug. The asymmetry is what makes it clearly a bug rather than a judgement call:
Two regression tests: one pinning the spaced form, one asserting the two forms agree — since the defect is precisely that they disagreed, and either alone would be unfalsifiable. I reviewed this function myself earlier and called the boundary logic correct. I checked the 2.
|
| URL | Result |
|---|---|
d-morrison/gha/pull/34 |
301 → Morrison-Lab/… ✅ |
d-morrison/gha/issues/325 |
404 ❌ |
Morrison-Lab/gha/issues/336 |
200 ✅ |
That is exactly why links / link-checker is red on main over 17 such URLs, tracked in #353. So the old-org URL form is not an equivalent-but-inconsistent alternative; it is broken, which is what commit 0bde33e fixed.
4. Self-contradictory claim about the opt-out selftest step — Addressed
Correct, and the contradiction is inside a single paragraph: CLAUDE.md states that an undeclared composite input is only an Actions warning, then two sentences later credits the step with pinning "that action.yml parses with the input declared." It can't — the step is green either way.
Both sites now say what the step actually covers (that action.yml parses, and that the opt-out path runs to completion) and point at what really pins declaration: test_declared_clause_breaks_default_matches_script_default, which reads each YAML file for the input's default: and fails outright when there is none.
Verification
- 62 tests pass (was 58).
- Each of the three new assertions was confirmed to fail against the defect it guards — I reverted
strip()→rstrip()and re-introduced the silent fallback in turn, watched the right tests fail, then restored. The unset-stays-silent guard passes either way by design. check-new-line-breaksrun againstorigin/mainover this branch's own added lines: clean (dogfooding the check this PR changes).- Added lines: no em-dashes, en-dashes, curly quotes, or
×; no new old-org issue URLs. _selftest.ymlparses.
Expect links / link-checker to stay red — that's #353 on main, not this diff.
Generated by Claude Code
2026-07-28 22:03 UTC |
|
Claude finished review — View run Code Review — #337This PR has been through five review rounds already. I re-verified the current head ( Prior findings: all confirmed fixedI checked each of the seven findings from earlier rounds (plus the four from the most recent round) against the actual current file content, not just the PR's own summary of what it fixed:
Two independent Opus bug-scan passes (adversarial regex probing, catastrophic-backtracking checks, an empirical run of the clause check against this repo's own New findings this round (all minor, non-blocking)
No VerdictReady for merge. All previously-identified real bugs are fixed and verified against the current code; the only remaining items are three minor, non-blocking style/consistency nits with no functional impact. |
Round 5 --- disposition of all 11 open threadsPicking this back up after the parallel session released it. Its closing Head is now @dem-extra1's findings --- fixed by the parallel session, verified hereI re-ran your own repros against the current head rather than reading the
Your second finding was the sharpest thing in this PR's whole review history: Wording, env-reader, four-sites-of-rationale, spec-citation, and selftest The four later findings --- verified and fixed in
|
Closes #336
What
check-new-line-breaksgains a second check: a newly-added line that joinstwo independent clauses with a semicolon, past a length gate.
That is a narrow slice of the SemBr spec's rule 5 (the
SHOULD), alongside the rule 4 MUST the check already enforced.
It is on by default, per the maintainer's direction that a useful feature
should be opt-out rather than opt-in.
clause-breaks: falseturns it off;clause-min-length(default'80')moves the gate.
Note this reverses #336's own proposal, which asked for opt-in.
The measurement supports the reversal: #336 argued against a blanket
punctuation rule at a 50.5% flag rate, but this narrow check flags 0.7%
of already-conforming lines.
Defaulting it on is also safe because the check stays warn-only unless
fail: trueis set, so it adds annotations rather than build failures.Why semicolons only, and why a length gate
The spec's rule 5 names a comma, semicolon, colon, and em dash, but as the
punctuation that marks the end of an independent clause -- not as the
trigger.
Measured over a 22,820-line conformant corpus:
,;:--;only, no length gate;past the length gate (this PR)A comma is overwhelmingly a list separator or an appositive; a colon usually
introduces a list, which rule 7 already breaks before; a dash is usually a
paired parenthetical, where breaking at the first but not the second is wrong.
The rationale lives in
has_unbroken_clause's docstring so a future readerfinds it next to the code rather than in this thread.
Review rounds
Round 1 --- two findings, both real:
stripped one, so a 128-character line with 16 characters of visible prose
behind a URL was flagged. That also broke the URL-inflation exception in
ai-config's own semantic-line-breaks guidance.
does cover
.md(the enforcement gap is check-non-standard-chars skips .md files (and U+00D7), so gha's own README has 39 unflagged em-dashes #322), but the aside explained theexclusion of a character the predicate has no branch for, so it is gone.
Round 2 --- five findings, all real:
bug for unbracketed links.
strip_inline_markupnow runs five patterns,ordered so link targets collapse before the bare-URL pattern runs.
;too, and were not stripped.the
CLAUDE.mdparagraph I added could claim it proved the input reaches thescript. Replaced by real
env var -> main() -> exit codecoverage; bothclaims corrected.
clause-min-lengthwastype: number, against this repo's idiom forlength-threshold inputs forwarded to a composite. Now
type: string.find_violationsonly calledclassify_line. Deleted;the exit-code tests cover the plumbing it claimed to.
Verification
rounds was confirmed to fail against the defect it guards before being
kept --- including stubbing the env read to prove the exit-code tests catch
an input that never arrives.
d-morrison/ai-config, thedefault-on run and the
clause-breaks: falserun differ, so the toggledemonstrably reaches the script. Its first real catch was a genuine
semicolon-joined clause in ai-config's own
CLAUDE.md.all fixed, and the check now reports clean.
strip_inline_markupdocuments its invariant: it only ever removescharacters, so visible length is a conservative lower bound, and under-counting
can only suppress a flag rather than invent one.
Defaults are declared in three places, and a test pins them
_DEFAULT_CLAUSE_BREAKS/_DEFAULT_CLAUSE_MIN_LENGTHare the single source;action.ymland the reusable workflow re-declare them.A parametrized test reads both YAML files and asserts they agree with the
script --- the #303 precedent.
It earned its place immediately: the first draft left
find_violations()onFalsewhile everything else had moved toTrue.Doc sync
All four sites plus the caller stub and a
changelog.dfragment (categorychanged, since behavior changes for existing consumers with no action ontheir part).