fix(repo-hygiene): treat trailing blank as success in batch_read_lines_into

[kyle-sexton]

Closes #3482

Summary

batch_read_lines_into treated a trailing blank line as a read failure, so clean-batch.sh reported file not found for a valid file ending in a\n\n.

Fix

The helper's exit status is now a source-open verdict: 0 for ordinary EOF (including trailing blanks), 1 only when the named source is missing, not regular, or unopenable. Both batch CLIs trust that contract. Plugin 0.10.20.

Verification

scripts/affected-tests.sh --run passed the three covering suites. Direct helper and both-CLI regressions cover trailing blanks, missing files, and unopenable files.

Related

N/A

  

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-02T07:11:23.815340Z	011bd31	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 1m 9s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Apply skip gate
• Review changed files for security-relevant surface
• Post findings / no-issues summary

No security issues found.

This PR changes the exit-status contract of batch_read_lines_into in plugins/repo-hygiene/skills/clean/scripts/lib/batch-common.sh so a trailing blank line no longer maps to "file not found," and updates the two callers (clean-batch.sh, git-tree-reset-batch.sh) plus tests accordingly. All affected paths are local CLI tooling operating on files/paths the invoking user already supplies on the command line — there's no network input, no privilege boundary, no secrets handling, and no GitHub Actions workflow changes in this diff.

Reviewed the file-descriptor handling specifically (exec {fd}<"$src" at batch-common.sh#L55): the path is always quoted, there's no eval/unquoted expansion introduced, and the check-then-open sequence ([[ -f "$src" ]] → exec {fd}<"$src") has a theoretical TOCTOU window, but since $src is a path the same local user already controls (no cross-privilege or cross-user boundary here), it isn't an exploitable finding.

[github-actions]

Last security-reviewed head: 011bd31b00398b0de61fe3563873035f46b2ea55. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

Claude finished @kyle-sexton's task in 3m 40s —— View job

---

Code review complete

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Review changed files for correctness / maintainability
• Post findings

No blocking findings.

What I checked

The core fix in batch_read_lines_into (plugins/repo-hygiene/skills/clean/scripts/lib/batch-common.sh#L40-64) correctly separates the "source opened" verdict from the "last read at EOF" verdict:

• The stdin branch (src == "-") is unchanged by this diff and already unconditionally returned success — not part of this fix.
• The named-source branch now does an explicit [[ -f "$src" ]] || return 1 pre-check, an explicit exec {fd}<"$src" || return 1 open (so a permission-denied-but--f-true file is still caught, since -f doesn't imply readability), and an explicit trailing return 0 so the loop's last read/[[ -n "$line" ]] exit status (false on a trailing blank) can no longer leak out as the function's result. The exec {fd}<&- close is reached on every path that opened the fd, so no descriptor leak.
• Both CLIs (clean-batch.sh, git-tree-reset-batch.sh) now chain batch_read_lines_into ... || fail_usage "file not found: $2" for both --repos-from and --skip-from, consistent with the new contract. git-tree-reset-batch.sh's removed -f pre-check is subsumed by the helper's own check, and since the script runs under set -uo pipefail (no -e), the old code's un-chained call wasn't silently broken by the pre-fix bug — this change is a coordinated tightening, not a hidden behavior loss.
• Test coverage (batch-common.test.sh, clean-batch.test.sh, git-tree-reset-batch.test.sh) exercises trailing blank, empty file, no-trailing-newline, CR-terminated, missing, non-regular (directory), and unopenable (chmod 000, with a CAP_DAC_OVERRIDE-aware skip) sources for both rc and array contents. The matrix matches the contract documented in the new comment block.
• CHANGELOG.md and plugin.json (0.10.19 → 0.10.20) are updated consistently with the fix.

One pre-existing (not introduced by this PR) behavioral quirk noted but not flagged as a finding: --skip-from - is undocumented for stdin (only --repos-from FILE|- advertises -), yet batch_read_lines_into's stdin special-case would still accept it. That branch is untouched by this diff, so it's out of scope here per the review criteria (pre-existing issue on untouched lines).

Verification note

I could not execute the test suite or scripts/affected-tests.sh --run myself in this lane (only gh pr diff/view/comment/review + read-only file tools are available here), so the PR description's "3 covering suites passed" is author-claimed and unverified by me. My review is based on static reading of the diff and cross-referencing the new tests against the fixed logic, which line up correctly.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 011bd31b00

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @cursor[bot]'s task in 4m 45s —— View job

---

Code review complete

• Gather context (PR diff at HEAD 002bda34, prior review history, CLAUDE.md/skill criteria)
• Apply skip gate (PR open, not draft; head changed since the last lane review — the newest commit swapped exec {fd}<"$src" for exec 3<"$src" to address Codex's fd-exhaustion finding — so re-review proceeded)
• Review changed files for correctness / maintainability
• Post findings

What changed since the last review

The prior Claude/Codex/security reviews covered commit 011bd31. The current head (002bda34) makes exactly one substantive change on top of that: batch-common.sh#L54-57 now opens the named source on a fixed descriptor (exec 3<"$src") instead of exec {fd}<"$src", per Codex's P2 review comment that Bash's {fd} allocator only searches fd ≥10, so a ulimit -n 10 runner failed to open even a perfectly readable list file. Fixed fd 3 sits safely under that limit (0/1/2 + 3 = 4 descriptors). CHANGELOG.md was updated to describe this.

I traced the fd-3 choice for collisions with the rest of this script family: clean_apply_manifest in clean-common.sh also uses fd 3, but only as a done 3<"$manifest" loop-scoped redirect, and it's never active while batch_read_lines_into runs (the latter only runs during early CLI arg parsing, before any per-repo manifest processing). Neither batch CLI (clean-batch.sh, git-tree-reset-batch.sh) opens fd 3 anywhere else. No collision.

Finding (posted inline)

One test-coverage gap, posted as an inline comment on batch-common.sh:57: the fix's whole reason for existing — behavior under a low ulimit -n — has no regression test anywhere in the three updated test files, even though every other input shape (trailing blank, empty, CR, no-trailing-newline, missing, non-regular, unopenable) got a dedicated case. A future refactor could reintroduce an fd ≥10 allocator and nothing would catch it.

Not flagged (considered, didn't meet the bar)

• The switch from a loop-scoped done <"$src" redirect to an explicit exec 3<"$src" / exec 3<&- pair is slightly more fragile in principle (an early return added later would leak fd 3), but the function has no such return path today, and this exact pattern already survived two prior close-reading review passes on the predecessor {fd} version without complaint — not high-signal enough to re-raise.
• A fixed fd 3 could in principle collide with a descriptor a caller already had open before invoking the script — narrower and less likely than the ulimit -n 10 problem it fixes, and the code comment already documents the tradeoff. Not flagged.

Verification note

I did not execute scripts/affected-tests.sh or the shell test suites myself in this lane (only gh pr diff, Read/Glob/Grep, and the two GitHub comment tools are available here); the PR description's test-pass claim is author-claimed and unverified by me.