fix: use grep -E + sed instead of grep -P in sync-compat.sh for macOS compatibility - #49127
Conversation
…nc-compat.sh Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49127 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Updates compatibility syncing to avoid GNU-only grep -P.
Changes:
- Replaces Perl-regex extraction with
grep -Eandsed -E. - Anchors matching to exclude commented examples.
Show a summary per file
| File | Description |
|---|---|
scripts/sync-compat.sh |
Revises Copilot version extraction for macOS compatibility. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
|
|
||
| # Extract DefaultCopilotVersion from Go constants file | ||
| COPILOT_VERSION=$(grep -oP '^\s*const DefaultCopilotVersion\s+Version\s*=\s*"\K[^"]+' "$CONSTANTS_FILE") | ||
| COPILOT_VERSION=$(grep -E '^\s*const DefaultCopilotVersion' "$CONSTANTS_FILE" | sed -E 's/.*"([^"]+)".*/\1/') |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Approve
Single-line, well-scoped fix. Verified the new grep -E ... | sed -E pipeline correctly extracts 1.0.75 from pkg/constants/version_constants.go and correctly skips the commented-out example line (// const DefaultCopilotVersion ...) thanks to the ^ anchor. No correctness, security, or maintainability issues found in the diff.
Analysis notes
- Tested the exact replacement command against the current constants file; it returns the correct version.
- The
^\s*constanchor properly excludes the//-prefixed doc-comment example, preserving the original script's intended behavior. - No test coverage exists for this script either before or after the change, but that's pre-existing and out of scope for this minimal portability fix.
🔎 Code quality review by PR Code Quality Reviewer · auto · 13.9 AIC · ⊞ 7.8K
Comment /review to run again
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix correctly replaces grep -P (Perl regex, unavailable on macOS) with a portable grep -E | sed pipeline. The sed pattern accurately extracts the version string. Change is minimal, correct, and POSIX-compatible.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 9.92 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix is correct and well-scoped.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed: BSD grep on macOS does not support
-P; replacing withgrep -E | sedis the right POSIX-compatible approach. ^anchor is essential: The commented example line inversion_constants.go(// const DefaultCopilotVersion Version = "0.0.369") would have matched without the anchor, returning the wrong version. The PR description correctly calls this out.- Missing regression test: There is no shell-based test that verifies
sync-compat.shextracts the correct version. If the regex ever regresses, CI on Linux would still pass silently.
Positive Highlights
- ✅ Minimal, targeted one-line change — no collateral changes
- ✅ Good PR description explains both the macOS compat issue and the
^anchor rationale - ✅
sed -Eused consistently, avoiding GNU-only extensions
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 24.2 AIC · ⊞ 7K
Comment /matt to run again
|
🎉 This pull request is included in a new release. Release: |
make buildfails on macOS because BSD grep doesn't support-P(Perl regex). Thesync-compat.shscript usedgrep -oPto extractDefaultCopilotVersionfrom the Go constants file.Changes
scripts/sync-compat.sh: Replacegrep -oP '...\K[^"]+'withgrep -E '^\s*const DefaultCopilotVersion' | sed -E 's/.*"([^"]+)".*/\1/'^anchor is required to exclude the commented-out example line (// const DefaultCopilotVersion ...) which would otherwise be matched and return the wrong version