Skip to content

test(scripts): capture before grep in harness-jira-test to stop SIGPIPE flakes - #1164

Open
waynesun09 wants to merge 1 commit into
mainfrom
fix-harness-jira-test-sigpipe
Open

test(scripts): capture before grep in harness-jira-test to stop SIGPIPE flakes#1164
waynesun09 wants to merge 1 commit into
mainfrom
fix-harness-jira-test-sigpipe

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

scripts/harness-jira-test.sh runs under set -euo pipefail, and its positive assertions were jira_overlay_field … | grep -qF KEY. grep -q exits on the first match while yq is still writing the rest of a multi-line result, so yq takes SIGPIPE (rc 141) and pipefail turns the assertion false. Only the positive multi-line greps flake, which matches the observed failing set exactly (triage-jira-token-in-runner, triage-jira-email-in-sandbox, triage-jira-base-url-in-sandbox, code-jira-email-in-sandbox); the negative *-not-in-sandbox checks read to EOF and never did. The harness files are correct.

Changes

  • Add jira_overlay_has <harness> <expr> <needle>: captures the field into a variable and greps a here-string, so there is no pipe to break. The 14 piped call sites use it; the two grep -qF <pattern> <file> sites (no pipe) are untouched.

Testing

  • 12 consecutive runs of bash scripts/harness-jira-test.sh, all exit 0 (before: 2–4 failures per run, different set each time).
  • shellcheck clean on the file; make lint 0/0.

harness-jira-test.sh reported 2 to 4 failures per run on an unmodified
tree, and a different set each time: triage-jira-token-in-runner,
triage-jira-email-in-sandbox, triage-jira-base-url-in-sandbox and
code-jira-email-in-sandbox. The Jira harness config was never wrong.

Every one of those assertions had the shape

  jira_overlay_field "${HARNESS}" "<expr>" | grep -qF "KEY"

under the script's `set -euo pipefail`. `grep -q` exits at its first
match, so when the key is early in yq's multi-line output yq is still
writing and takes SIGPIPE; pipefail then reports the successful match as
a failed condition. Instrumented with PIPESTATUS, the JIRA_TOKEN check
returned `rc=141 pipestatus=[0]` on 6 of 8 runs — 141 is 128+13, SIGPIPE
— while the yq expression alone printed JIRA_TOKEN every time.

That explains the exact failure set. Positive greps whose match is early
in multi-line output flake; the negative *-not-in-sandbox checks never
did, because a non-matching grep reads to EOF; and the single-line
.providers[] and .openshell.profiles[] checks never did either. The race
also needs suite contention: the same pipeline passed 40 out of 40 runs
in isolation.

Add jira_overlay_has, which captures the field into a variable and greps
a here-string, and route the fourteen piped call sites through it. The
script now passes 12 consecutive runs.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@waynesun09
waynesun09 requested a review from a team as a code owner September 3, 2026 12:55
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Stabilize Jira harness assertions against SIGPIPE

🐞 Bug fix 🧪 Tests 🕐 Less than 10 minutes

Grey Divider

AI Description

• Prevent flaky Jira harness assertions caused by SIGPIPE under pipefail.
• Centralize fourteen overlay checks through captured-output literal matching.
Diagram

graph TD
  A["Harness assertions"] --> B["jira_overlay_has"] --> C["jira_overlay_field"] --> D["Captured output"] --> E{"Literal found?"}
  E -->|Yes| F["Pass branch"]
  E -->|No| G["Fail branch"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Drain grep input without quiet mode
  • ➕ Preserves streaming without buffering command output.
  • ➕ Avoids SIGPIPE because grep reads through end-of-file.
  • ➖ Retains pipeline behavior under pipefail.
  • ➖ Requires output redirection and is less explicit about preventing early termination.

Recommendation: Keep the captured-output helper. Although grep -F ... &gt;/dev/null could drain the pipeline safely, centralizing extraction and matching explicitly removes the producer-consumer race, keeps call sites concise, and has negligible memory cost for small harness fields.

Files changed (1) +26 / -14

Bug fix (1) +26 / -14
harness-jira-test.shAvoid SIGPIPE races in Jira overlay assertions +26/-14

Avoid SIGPIPE races in Jira overlay assertions

• Adds 'jira_overlay_has' to capture complete 'yq' output before fixed-string matching through a here-string. Routes fourteen piped Jira overlay assertions through the helper so 'grep -qF' cannot terminate 'yq' early under 'set -o pipefail'.

scripts/harness-jira-test.sh

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. Protected script path modified 📜 Skill insight § Compliance
Description
The PR modifies scripts/harness-jira-test.sh, which is under the protected scripts/
governance/infrastructure path. This requires human review and must not be auto-approved; the PR
description does not provide a linked issue or explicit authorization for the protected-path change.
Code

scripts/harness-jira-test.sh[R43-47]

+jira_overlay_has() {
+  local out
+  out="$(jira_overlay_field "$1" "$2")"
+  grep -qF "$3" <<< "${out}"
+}
Relevance

●●● Strong

Protected-path governance findings are accepted for scripts changes requiring explicit human review
or authorization.

PR-#569

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The checklist explicitly identifies scripts/ as a protected path and requires a finding whenever a
protected file is modified. The diff adds a helper and changes harness assertions in
scripts/harness-jira-test.sh, while the PR information supplies no linked issue or separate
authorization.

scripts/harness-jira-test.sh[37-47]
Skill: pr-review

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

## Issue description
This PR modifies a file under the protected `scripts/` path, which requires explicit human review and authorization.

## Issue Context
The compliance rule requires a finding for protected-path changes and treats changes without a linked issue or explanation as high severity.

## Fix Focus Areas
- scripts/harness-jira-test.sh[37-47]

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


Grey Divider

Context sources
✅ Compliance rules (platform): 57 rules
✅ Skills: 4 invoked
  code-review
  code-implementation
  pr-review
  docs-review
Review mode: 🚀 Fast: This is a localized shell-test harness fix that removes a known SIGPIPE race without changing production behavior or security-sensitive logic.

Grey Divider

Tip of the day
💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +43 to +47
jira_overlay_has() {
local out
out="$(jira_overlay_field "$1" "$2")"
grep -qF "$3" <<< "${out}"
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Protected script path modified 📜 Skill insight § Compliance

The PR modifies scripts/harness-jira-test.sh, which is under the protected scripts/
governance/infrastructure path. This requires human review and must not be auto-approved; the PR
description does not provide a linked issue or explicit authorization for the protected-path change.
Agent Prompt
## Issue description
This PR modifies a file under the protected `scripts/` path, which requires explicit human review and authorization.

## Issue Context
The compliance rule requires a finding for protected-path changes and treats changes without a linked issue or explanation as high severity.

## Fix Focus Areas
- scripts/harness-jira-test.sh[37-47]

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

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 3, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:56 PM UTC · Completed 1:09 PM UTC

Commit: 39f8024 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.37

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 3, 2026
@fullsend-ai-review

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Small, single-file test script fix by a known contributor with full test coverage, modestly elevated by one protected path and the file being a recent churn hotspot with multiple authors and prior fix commits in the 90-day window.

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

High

  • [protected-path] scripts/harness-jira-test.sh — This PR modifies a file under the scripts/ protected path. The PR has no linked issue providing authorization for modifying governance or infrastructure files. Human approval is required for protected-path changes regardless of context.
    Remediation: Obtain human reviewer approval for changes to protected paths.

Low

  • [missing-authorization] scripts/harness-jira-test.sh — No linked issue. The PR has no closing issue reference, the body does not mention an issue number, and the branch name contains no issue identifier. While the change is a straightforward test-flake fix with no production impact, traceability to a tracking issue is a good practice.
    Remediation: Create a tracking issue describing the SIGPIPE flake and link it to this PR, or add a comment explaining why no issue is needed for this class of fix.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

@waynesun09

Copy link
Copy Markdown
Member Author

Tracking issue for the flake: #1165 (test-only fix, no production impact; protected-path approval is a human call).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant