fix(pre-fetch): extract the prior SHA with sed, not BSD-incompatible grep -P

[guyoron1]

Heyaaaa : )

Summary

pre-fetch-prior-review.sh extracts the prior review SHA with grep -oP. BSD grep (macOS) has no -P, the || true swallows the failure, and PRIOR_SHA comes back empty — two script tests fail on every macOS checkout. Replaced with sed -nE, the same expression in POSIX ERE. Second commit applies the identical swap to the GitLab scaffold twin (its comment declares it "equivalent" to this script) so the pair stays in lockstep — caught in review by @waynesun09.

Related Issue

None — found running the script test suite on macOS.

Changes

• sed -nE 's/.*\*\*Head SHA:\*\* ([0-9a-f]{7,64}).*/\1/p' | head -1 replaces the grep; the || true goes too, since a sed no-match exits 0.
• Same swap at fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:462, with a lockstep pointer comment.
• One deliberate edge change: two **Head SHA:** markers on one line now yield the last, not the first. Across lines — the only shape the pipeline writes — behavior is identical, and the tests cover it.

Sweep status: hack/lint-docs-links had the same bug class and is macOS-reachable via pre-commit — fixed in #6728. The guard at reconcile-repos.sh:208 is inert on macOS (BSD grep exits non-zero, the if reads that as false) but harmless: the anchored allowlist at :212 already rejects control characters. Still unswept, genuinely Linux-only: scripts/renovate/*, hack/gitlab-runner-vm/setup.sh.

Testing

• make lint passes (stage changes first, then run)
• Tests added/updated for new or modified logic

pre-fetch-prior-review-test.sh: 2 failures → all pass on macOS. GitLab twin pipeline run on the same fixtures, identical output. CI (GNU grep) behaviorally unchanged.

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• Commits are signed off (DCO) — human and human-directed agent sessions only
• I wrote this contribution myself and can explain all changes in it

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[qodo-code-review]

PR Summary by Qodo

Make prior-review SHA extraction portable across macOS and Linux

🐞 Bug fix 🕐 Less than 10 minutes

AI Description

• Replace GNU-only PCRE extraction with portable POSIX ERE parsing.
• Preserve empty-SHA handling without suppressing command failures.
• Restore existing prior-review script tests on macOS.

Diagram

graph TD
  A["Prior review body"] --> B["Current section"] --> C["Portable SHA parser"] --> D["Prior SHA output"]

Loading

High-Level Assessment

The sed-based POSIX ERE pipeline is the best fit because it works with both BSD and GNU userlands, retains the established shell pipeline, and naturally returns success on no match. Adding a Perl dependency or retaining grep with platform detection would add unnecessary complexity.

Files changed (1) +7 / -1

Bug fix (1) +7 / -1

pre-fetch-prior-review.shUse portable sed for prior-review SHA extraction +7/-1

Use portable sed for prior-review SHA extraction

• Replaces GNU grep's unsupported-on-macOS PCRE lookbehind with a POSIX ERE sed substitution. Removes the error-swallowing fallback because sed safely emits no output when no SHA matches.

internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review.sh

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Multiple matches abort pre-fetch 🐞 Bug ☼ Reliability

Description

With pipefail, head -1 can close the pipe while sed is still emitting matches, causing sed
to exit on SIGPIPE and the PRIOR_SHA assignment to terminate the script. A sufficiently large
prior-review section with repeated Head SHA lines therefore prevents the later review-agent step
from running.

Code

internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review.sh[R102-103]

    PRIOR_SHA="$(echo "${CURRENT_SECTION}" \
-        | grep -oP '(?<=\*\*Head SHA:\*\* )[0-9a-f]{7,64}' | head -1 || true)"
+        | sed -nE 's/.*\*\*Head SHA:\*\* ([0-9a-f]{7,64}).*/\1/p' | head -1)"

Relevance

●●● Strong

Recent accepted precedent flags unguarded grep/head pipelines under pipefail; this is the same
reliability failure mode.

PR-#390
PR-#2456

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script enables set -euo pipefail, accepts and processes prior bodies up to 1 MB, and imposes
no limit on matching lines. The changed pipeline is unguarded; its exported value feeds a workflow
step that executes before the review agent, so status 141 from sed aborts the review run.

internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review.sh[11-11]
internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review.sh[78-89]
internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review.sh[94-104]
.github/workflows/reusable-review.yml[143-180]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The unguarded `sed | head -1` pipeline can fail with SIGPIPE under `set -o pipefail` when many lines match, aborting pre-fetch before the review agent runs.

## Issue Context
Keep BSD/macOS portability and preserve successful empty output when no SHA exists. Prefer making `sed` print the first match and quit itself, eliminating `head`, rather than broadly swallowing genuine sed failures.

## Fix Focus Areas
- internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review.sh[102-103]
- internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh[121-179]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 69 rules

✅ Skills: writing-user-docs, writing-adrs

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[waynesun09]

Review sweep at head 0ce3f82. Both findings land on files outside this PR's diff (the diff is only pre-fetch-prior-review.sh), so they are here in the review body rather than as inline comments. Neither blocks the fix itself — the sed -nE change is correct and the right call.

---

MEDIUM — The GitLab scaffold twin keeps the byte-identical grep -oP, and the PR body's "five other scripts" sweep is understated

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:462

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:462 runs:

PRIOR_REVIEW_SHA=$(printf '%s' "${CURRENT_SECTION}" \
  | grep -oP '(?<=\*\*Head SHA:\*\* )[0-9a-f]{7,64}' | head -1 || true)

That is byte-identical to the line this PR replaces, including the surrounding awk '/<!-- sticky:history-start -->/{exit}' section split and the 1 MB MAX_REVIEW_BYTES cap. The comment at lines 390-394 calls it out explicitly: "Pre-fetch prior review for the review agent — equivalent to pre-fetch-prior-review.sh in the GitHub scaffold." After this PR the two self-declared-equivalent extractors diverge in exactly the multi-marker-on-one-line case the PR body flags as a deliberate behavior change (grep takes the first, sed's greedy .* takes the last).

Separately, the PR body's sweep enumeration is wrong. It says "five other scripts use grep -P (scripts/renovate/*, hack/gitlab-runner-vm/setup.sh, reconcile-repos.sh). They are Linux-only automation and out of scope." A repo-wide search returns two sites that enumeration misses. The GitLab twin above is one. The other is hack/lint-docs-links:32:

done < <(grep -oP '(?<=\])\(\K[^)]+' "$mdfile" || true)

which is not Linux-only automation: .pre-commit-config.yaml:156-161 wires it as the lint-docs-links pre-commit hook over ^docs/.*\.md$, so it runs on developer machines, macOS included, with the same fail-open || true this PR is fixing. On a macOS checkout it yields zero links and the docs-link scope check passes vacuously. That is the same bug class on the same platform the PR exists to fix, described in the PR body as out of scope.

Suggestion: Apply the same sed -nE extraction at fullsend-agent.yml:462 so the pair the comment calls "equivalent" stays equivalent (one line), or leave it and note at that line why it is deliberately divergent. Either way correct the PR body: the sweep is seven sites, not six, and hack/lint-docs-links is a macOS-reachable pre-commit hook rather than Linux-only automation — worth its own follow-up issue, since it fails open exactly the way this PR's target did.

---

MEDIUM — PR body's reconcile-repos.sh:208 claim is factually wrong; the anchored allowlist three lines later already rejects control characters

internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh:208

The PR body asserts as fact: "reconcile-repos.sh:208 deserves its own look — its grep -qP '[\x00-\x1f]' control-character guard does not error out on BSD, it just evaluates false, so that check quietly passes everything on a macOS run."

Reading the file at head 0ce3f82: line 208 is if printf '%s' "$name" | grep -qP '[\x00-\x1f]'; then inside validate_repo_name, and line 212 is if ! [[ "$name" =~ $REPO_NAME_PATTERN ]]; then with REPO_NAME_PATTERN='^[a-zA-Z0-9._-]+$' defined at line 30. That is an anchored allowlist that rejects every byte in \x00-\x1f regardless of what the grep on 208 returns, so "that check quietly passes everything on a macOS run" is false — the grep is defense-in-depth on top of a check that already covers its entire input class, and the macOS exposure is nil.

The mechanism claim is also imprecise: BSD grep does not succeed on -P, it exits non-zero with a usage error; the reason set -e does not fire is that the call sits in an if condition, where a non-zero status simply reads as false. Same net effect, different cause, and the difference matters to whoever follows the pointer — they will find stderr noise, not silence. Shipping "quietly passes everything" in a merged PR description invites someone to file a security issue against code that is already safe.

Suggestion: Reword to something checkable, e.g. "BSD grep exits non-zero on -P; inside the if that reads as false, so the guard at line 208 is inert on macOS — harmless in practice because the anchored REPO_NAME_PATTERN allowlist at line 212 already rejects control characters. Worth cleaning up for hygiene, not a hole."

[guyoron1]

Both fixed, thanks for the sweep check — both catches were right.

• GitLab twin: same sed -nE in c3babba, with a lockstep comment pointing at the GitHub script for the rationale.
• reconcile-repos.sh: PR body corrected per your wording — inert guard, input class already covered by the anchored allowlist; hygiene, not a hole.
• lint-docs-links: indeed macOS-reachable, and it was hiding a second bug — with links actually flowing, realpath -m is next in the loop and BSD realpath has no -m, so a grep-only fix would have hard-failed every macOS docs commit. Both fixed together in fix(hack): make lint-docs-links actually run on macOS #6728 (extraction parity verified across the docs corpus: 2798 links, zero mismatches; the old pipeline extracts 0 on macOS).