Skip to content

fix(ci): correct sed -Ei/--in-place boundary comment and extend shipped-list coverage - #1549

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/1548-review-comment-wording
Jul 26, 2026
Merged

fix(ci): correct sed -Ei/--in-place boundary comment and extend shipped-list coverage#1549
kyle-sexton merged 1 commit into
mainfrom
fix/1548-review-comment-wording

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

This was generated by AI during work-loop execution.

No linked issue

Summary

Test plan

  • bash scripts/check-shell-portability.test.sh — 75/75 passing (unchanged pass count; this
    PR only edits comments and extends one assertion's input set from 4 to 6 lines).
  • scripts/check-shell-portability.sh origin/main run directly against this branch's own
    diff — clean (no unexcused GNU-only constructs in the 1 changed file).
  • shellcheck --rcfile=.shellcheckrc scripts/check-shell-portability.test.sh — clean.
  • typos --config _typos.toml scripts/check-shell-portability.test.sh — clean.

Related

Two non-blocking findings from the automated PR review:

- The operator-terminated regression-case comment said "or quote must still
  be detected", which is exactly backwards for this token pair -- quotes are
  deliberately excluded from the boundary (that's the whole point of the
  narrower boundary vs. the sibling sort -V/grep -P/echo -e tokens). Fixed
  the wording and pointed at the "sed -Ei'' must not be flagged" test that
  explains why. Also corrected a stale claim in the shipped-list assertion's
  comment that these two tokens share the sibling tokens' boundary -- they
  deliberately do not.
- The shipped-list assertion proved 4 of the 6 operator-terminated forms
  (missing the `;`-terminated pair); the isolated-token loops already covered
  `;`, so this was not a correctness gap, but added the two missing cases so
  the shipped-list check is the exhaustive real-token-file proof it's meant
  to be.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


PR Review

  • Gather context and understand the PR
  • Review the diff (scripts/check-shell-portability.test.sh)
  • Verify comment accuracy claims
  • Verify shipped-list completeness claims
  • Post review findings

This PR is clean. The two changes are accurate and well-motivated.

Comment accuracy (lines 402–407)

The old comment said "…or quote must still be detected", which was technically backwards. Inspecting the sed -Ei token's regex at line 313:

(^|[^[:alnum:]_])sed([[:space:]][^\n]*)?[[:space:]]-[A-Za-z]*E[A-Za-z]*i([[:space:]|&;()<>]|$)

The terminal boundary is ([[:space:]|&;()<>]|$) — quotes are deliberately absent. A quote immediately after -Ei produces the sed -Ei'' attached-empty-suffix shape, which is the dual-compatible deferred form preserved by #1513/#1534. The new comment correctly calls this out and cross-references the "sed -Ei'' must not be flagged" test at line 369.

The shipped-list assertion comment at lines 751–756 is likewise corrected: the old phrasing "widened to the same boundary … sort -V/grep -P/echo -e already use" implied parity, when in fact the sed tokens use a strictly narrower boundary (no quotes). The new wording spells out the deliberate difference.

Shipped-list completeness (lines 757–770)

The four pre-existing inputs exercise subshell-close ($(…)) and pipe (|) boundary forms. The two new inputs:

'sed -Ei; echo done'
'sed --in-place; echo done'

cover the semicolon ; form, which is present in both regexes' terminal character class. The isolated-token while loops (lines 408–420, 452–461) already covered ; via their heredoc inputs, so there is no correctness gap being closed — the PR description accurately frames this as exhaustiveness of the shipped-list proof rather than a fix.

Count update 4 → 6 and error message update are consistent with the new input set.

No issues found

No bugs, logic errors, security concerns, or test regressions. The pass count stays at 75/75 per the PR description.

Approve.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review

  • Gather PR diff
  • Read changed file
  • Analyze for security issues
  • Post findings

No security issues found.

The diff is confined to scripts/check-shell-portability.test.sh and consists entirely of:

  1. Comment rewording (lines 402–407 and 751–756) — no executable surface.
  2. Two additional hardcoded test-case strings ('sed -Ei; echo done' and 'sed --in-place; echo done') added to a printf '%s\n' argument list, plus a count assertion bump from 4 → 6.

Neither change introduces user-controlled input, a new execution surface, or a change to trust boundaries. The relevant helpers (tmpsh, one_token_list, scan_paths) have not been modified; their security posture (mktemp-based temp files, no user input, no shell word-splitting of untrusted data) is unchanged.

The two new string literals contain shell metacharacters (;, echo), but they are passed as a single-quoted argument to printf '%s\n', which writes them verbatim to a temp file — they are never evaluated by the test runner itself.

@kyle-sexton
kyle-sexton merged commit bb440d0 into main Jul 26, 2026
30 checks passed
@kyle-sexton
kyle-sexton deleted the fix/1548-review-comment-wording branch July 26, 2026 13:00
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.

1 participant